From b92228f6c22e4dfa190a24eb10b3d2a17e4f325d Mon Sep 17 00:00:00 2001 From: Xiyuan Chen <52963600+GareArc@users.noreply.github.com> Date: Fri, 30 Oct 2026 08:24:55 +0000 Subject: [PATCH] feat(openapi): workspace skill list, export, import and publish ops (#44941) --- api/constants/__init__.py | 2 + api/controllers/openapi/__init__.py | 31 +- api/controllers/openapi/_contract.py | 37 +- api/controllers/openapi/_errors.py | 73 +++ api/controllers/openapi/_models.py | 91 ++- api/controllers/openapi/_multipart.py | 12 +- api/controllers/openapi/app_run.py | 3 +- api/controllers/openapi/app_workflow.py | 12 +- api/controllers/openapi/auth/requirements.py | 12 + api/controllers/openapi/skills.py | 449 ++++++++++++++ api/controllers/openapi/workflow_events.py | 3 +- api/openapi/markdown/openapi-openapi.md | 245 ++++++++ api/services/skill_management_service.py | 553 +++++++++++++----- .../openapi/auth/test_requirements.py | 73 ++- .../controllers/openapi/test__contract.py | 31 +- .../controllers/openapi/test__multipart.py | 15 +- .../controllers/openapi/test_skills.py | 501 ++++++++++++++++ .../services/test_skill_management_service.py | 421 ++++++++++++- cli/test/fixtures/catalog.json | 100 ++++ .../generated/api/openapi/orpc.gen.ts | 167 +++++- .../generated/api/openapi/types.gen.ts | 266 +++++++++ .../generated/api/openapi/zod.gen.ts | 234 ++++++++ .../contracts/non-json-openapi-responses.md | 1 + skills/difyctl/SKILL.md | 1 + skills/difyctl/references/workspace-skills.md | 50 ++ 25 files changed, 3199 insertions(+), 184 deletions(-) create mode 100644 api/controllers/openapi/skills.py create mode 100644 api/tests/unit_tests/controllers/openapi/test_skills.py create mode 100644 skills/difyctl/references/workspace-skills.md diff --git a/api/constants/__init__.py b/api/constants/__init__.py index 15ff28bc417..9ad835bb89f 100644 --- a/api/constants/__init__.py +++ b/api/constants/__init__.py @@ -7,6 +7,8 @@ UUID_NIL = "00000000-0000-0000-0000-000000000000" DEFAULT_FILE_NUMBER_LIMITS = 3 +BYTES_PER_MIB = 1024 * 1024 + _IMAGE_EXTENSION_BASE: frozenset[str] = frozenset(("jpg", "jpeg", "png", "webp", "gif", "svg")) _VIDEO_EXTENSION_BASE: frozenset[str] = frozenset(("mp4", "mov", "mpeg", "webm")) _AUDIO_EXTENSION_BASE: frozenset[str] = frozenset(("mp3", "m4a", "wav", "amr", "mpga")) diff --git a/api/controllers/openapi/__init__.py b/api/controllers/openapi/__init__.py index ab4d7a9daa9..4d21fe04037 100644 --- a/api/controllers/openapi/__init__.py +++ b/api/controllers/openapi/__init__.py @@ -8,7 +8,7 @@ from controllers.common.access_response import register_auth_error_response # Register response/query models BEFORE importing controller modules so that # @openapi_ns.response / @openapi_ns.expect decorators can resolve model names. -from controllers.common.fields import EventStreamResponse, SimpleResultResponse +from controllers.common.fields import BinaryFileResponse, EventStreamResponse, SimpleResultResponse from controllers.common.schema import register_enum_models, register_response_schema_models, register_schema_models from controllers.openapi._catalog import attach_catalog from controllers.openapi._errors import ErrorBody, OpenApiErrorCode, OpenApiErrorFormatter @@ -125,6 +125,18 @@ from controllers.openapi._models import ( SessionListQuery, SessionListResponse, SessionRow, + SkillDescribeResponse, + SkillExportQuery, + SkillImportPayload, + SkillImportResponse, + SkillListQuery, + SkillListResponse, + SkillPublishPayload, + SkillPublishResponse, + SkillRow, + SkillVersionListQuery, + SkillVersionListResponse, + SkillVersionRow, TaskStopResponse, ToolCredentialCreatePayload, ToolCredentialUpdatePayload, @@ -245,12 +257,18 @@ register_schema_models( PublishPayload, RunListQuery, SessionListQuery, + SkillExportQuery, + SkillImportPayload, + SkillListQuery, + SkillPublishPayload, + SkillVersionListQuery, VersionListQuery, WorkflowRunPayload, WorkspaceListQuery, ) register_response_schema_models( openapi_ns, + BinaryFileResponse, ErrorBody, EventStreamResponse, SimpleResultResponse, @@ -343,6 +361,13 @@ register_response_schema_models( KnowledgeRetrievalFragment, LlmModelBlock, MultipleRetrievalFragment, + SkillRow, + SkillImportResponse, + SkillListResponse, + SkillVersionRow, + SkillDescribeResponse, + SkillVersionListResponse, + SkillPublishResponse, ToolProviderDetailResponse, ToolProviderRow, ) @@ -375,6 +400,7 @@ def _register_controllers() -> tuple[ModuleType, ...]: oauth_device, oauth_device_sso, plugins, + skills, tool_providers, workflow_events, workspaces, @@ -401,6 +427,7 @@ def _register_controllers() -> tuple[ModuleType, ...]: oauth_device, oauth_device_sso, plugins, + skills, tool_providers, workflow_events, workspaces, @@ -428,6 +455,7 @@ def _register_controllers() -> tuple[ModuleType, ...]: oauth_device, oauth_device_sso, plugins, + skills, tool_providers, workflow_events, workspaces, @@ -456,6 +484,7 @@ __all__ = [ "oauth_device", "oauth_device_sso", "plugins", + "skills", "tool_providers", "workflow_events", "workspaces", diff --git a/api/controllers/openapi/_contract.py b/api/controllers/openapi/_contract.py index 1c514598b80..653ecc736be 100644 --- a/api/controllers/openapi/_contract.py +++ b/api/controllers/openapi/_contract.py @@ -18,7 +18,7 @@ from __future__ import annotations import inspect from collections.abc import Callable, Sequence from functools import wraps -from typing import Any, Final, cast, override +from typing import Any, Final, cast, get_args, get_origin, override from flask import request from flask_restx import abort @@ -26,6 +26,7 @@ from pydantic import BaseModel, ValidationError from sqlalchemy.orm import Session from werkzeug.exceptions import RequestEntityTooLarge +from constants import BYTES_PER_MIB from controllers.common.schema import query_params_from_model, query_params_from_request from controllers.openapi import openapi_ns from controllers.openapi._errors import ErrorBody, RequestOverRouteLimits @@ -40,10 +41,22 @@ from controllers.openapi.auth.spec import AdmissionContext, CatalogMeta, Effect, from controllers.openapi.auth.subjects import Subject from enums import DeploymentEdition -__all__ = ["CapRequestBody", "Effect", "Example", "Kind", "accepts", "endpoint", "op_of", "paginated", "returns"] +__all__ = [ + "MULTIPART_HEADER_BYTES_PER_PART", + "CapRequestBody", + "Effect", + "Example", + "Kind", + "accepts", + "endpoint", + "multipart_header_allowance", + "op_of", + "paginated", + "returns", +] _INJECTED_KWARGS: Final = frozenset({"ctx", "query", "body"}) -_BYTES_PER_MIB: Final = 1024 * 1024 +MULTIPART_HEADER_BYTES_PER_PART: Final = 1024 def paginated(op: str) -> Callable: @@ -82,6 +95,10 @@ class CapRequestBody(Requirement): request.max_form_parts = self.max_parts() +def multipart_header_allowance(parts: int) -> int: + return parts * MULTIPART_HEADER_BYTES_PER_PART + + def _read_body(body_file_fields: frozenset[str]) -> dict[str, Any]: try: return body_from_request(file_fields=body_file_fields) @@ -91,21 +108,31 @@ def _read_body(body_file_fields: frozenset[str]) -> dict[str, Any]: raise raise RequestOverRouteLimits( f"{RequestOverRouteLimits.description}: at most {request.max_form_parts} parts" - f" and {limit / _BYTES_PER_MIB:g} MiB" + f" and {limit / BYTES_PER_MIB:g} MiB" ) from exc +def _list_fields(model: type[BaseModel]) -> tuple[str, ...]: + """Query fields typed as a list, which a repeated parameter fills.""" + return tuple( + name + for name, field in model.model_fields.items() + if any(get_origin(part) is list for part in (field.annotation, *get_args(field.annotation))) + ) + + def accepts(*, query: type[BaseModel] | None = None, body: type[BaseModel] | None = None) -> Callable: """Validate ``query``/``body`` against the models and inject them as keyword-only kwargs.""" def decorator(view: Callable) -> Callable: body_file_fields = file_fields(body) if body is not None else frozenset() + query_list_fields = _list_fields(query) if query is not None else () @wraps(view) def wrapper(*args: Any, **kwargs: Any) -> Any: try: if query is not None: - kwargs["query"] = query_params_from_request(query) + kwargs["query"] = query_params_from_request(query, list_fields=query_list_fields) if body is not None: kwargs["body"] = body.model_validate(_read_body(body_file_fields)) except ValidationError as exc: diff --git a/api/controllers/openapi/_errors.py b/api/controllers/openapi/_errors.py index 9c0edc1228a..c1b89e10a6e 100644 --- a/api/controllers/openapi/_errors.py +++ b/api/controllers/openapi/_errors.py @@ -27,6 +27,7 @@ import contextlib import logging from enum import StrEnum from typing import Any +from uuid import UUID from pydantic import BaseModel from werkzeug.exceptions import HTTPException @@ -107,6 +108,13 @@ class OpenApiErrorCode(StrEnum): DRAFT_CHANGED = "draft_changed" APP_MODE_MISMATCH = "app_mode_mismatch" SERVICE_API_DISABLED = "service_api_disabled" + SKILLS_DISABLED = "skills_disabled" + SKILL_NOT_FOUND = "skill_not_found" + SKILL_VERSION_NOT_FOUND = "skill_version_not_found" + SKILL_NAME_CONFLICT = "skill_name_conflict" + SKILL_NAME_MISMATCH = "skill_name_mismatch" + SKILL_PACKAGE_INVALID = "skill_package_invalid" + SKILL_REQUEST_REJECTED = "skill_request_rejected" class ErrorDetail(BaseModel): @@ -541,3 +549,68 @@ class ServiceApiDisabled(OpenApiError): # noqa: N818 error_code = OpenApiErrorCode.SERVICE_API_DISABLED description = "This app's Service API is turned off." hint = "A workspace admin can turn it on; hints holds the op." + + +class SkillsDisabled(OpenApiError): # noqa: N818 + code = 503 + error_code = OpenApiErrorCode.SKILLS_DISABLED + description = "Skills are turned off on this server." + hint = "Ask an admin to turn on Skills." + + +class SkillNotFound(OpenApiError): # noqa: N818 + code = 404 + error_code = OpenApiErrorCode.SKILL_NOT_FOUND + description = "No skill with this id exists in this workspace." + hint = "List them with get.skill." + + +class SkillVersionNotFound(OpenApiError): # noqa: N818 + code = 404 + error_code = OpenApiErrorCode.SKILL_VERSION_NOT_FOUND + description = "No version with this id exists for this skill." + hint = "List them with get.skill.version." + + +class SkillNameConflict(OpenApiError): # noqa: N818 + code = 409 + error_code = OpenApiErrorCode.SKILL_NAME_CONFLICT + description = "A skill with this name already exists in this workspace." + + +class SkillNameMismatch(OpenApiError): # noqa: N818 + code = 400 + error_code = OpenApiErrorCode.SKILL_NAME_MISMATCH + description = "The name in SKILL.md is not the name of the skill being overwritten." + + +class SkillPackageInvalid(OpenApiError): # noqa: N818 + code = 400 + error_code = OpenApiErrorCode.SKILL_PACKAGE_INVALID + description = "The skill folder is not a valid skill." + + +class SkillPackageTooLarge(OpenApiError): # noqa: N818 + code = 413 + error_code = OpenApiErrorCode.REQUEST_TOO_LARGE + description = "The skill folder is over a limit of this server." + + +class SkillRequestRejected(OpenApiError): # noqa: N818 + error_code = OpenApiErrorCode.SKILL_REQUEST_REJECTED + description = "The skill request was refused." + + def __init__( + self, message: str | None = None, *, status_code: int, details: list[ErrorDetail] | None = None + ) -> None: + super().__init__(message, details=details) + self.code = status_code + + +def require_uuid(value: str, *, not_found: type[OpenApiError]) -> str: + """Parse a path id as a UUID or raise 404; an unparsed id would otherwise reach + a UUID-typed database column and fail as a 500.""" + try: + return str(UUID(value)) + except ValueError as e: + raise not_found() from e diff --git a/api/controllers/openapi/_models.py b/api/controllers/openapi/_models.py index b2ea11a5fa4..77510b322b6 100644 --- a/api/controllers/openapi/_models.py +++ b/api/controllers/openapi/_models.py @@ -7,7 +7,7 @@ from datetime import datetime from enum import StrEnum from typing import Any, Final, Literal, Protocol, Self -from pydantic import BaseModel, ConfigDict, Field, PrivateAttr, field_validator, model_validator +from pydantic import BaseModel, ConfigDict, Field, PrivateAttr, ValidationInfo, field_validator, model_validator from constants.languages import supported_language from constants.oauth_bearer import SubjectType @@ -1330,3 +1330,92 @@ class ModelCredentialCreatePayload(ModelRef): class ModelCredentialUpdatePayload(ModelRef): credentials: dict[str, Any] = Field(description=_CREDENTIALS_UPDATE_DESCRIPTION) name: str | None = None + + +class SkillListQuery(PageQuery): + model_config = ConfigDict(extra="forbid") + + keyword: str | None = Field( + None, max_length=200, description="Words to match in the name, display name or description" + ) + tag: list[str] | None = Field(None, description="Only skills with every one of these tags; repeat to give several") + + +class SkillRow(BaseModel): + id: str + name: str + display_name: str + description: str + icon: str | None + tags: list[str] + updated_at: int + latest_published_version_id: str | None = Field(description="Null while the skill has never been published") + + +class SkillListResponse(PaginationEnvelope[SkillRow]): + pass + + +class SkillVersionRow(BaseModel): + id: str + version_number: int + version_name: str | None + publish_note: str + created_at: int + created_by: str | None = Field(validation_alias="published_by", description="Account id that published it") + + +class SkillDescribeResponse(SkillRow, Hinted): + draft_hash: str = Field(description="Opaque; names the draft this was read from") + latest_version: SkillVersionRow | None = Field(description="Null while the skill has never been published") + + +class SkillVersionListQuery(PageQuery): + model_config = ConfigDict(extra="forbid") + + +class SkillVersionListResponse(PaginationEnvelope[SkillVersionRow]): + pass + + +class SkillExportQuery(BaseModel): + model_config = ConfigDict(extra="forbid") + + version_id: UUIDStr | None = Field(None, description="Export this published version instead of the draft") + + +class SkillPublishPayload(BaseModel): + model_config = ConfigDict(extra="forbid") + + version_name: str | None = Field(None, max_length=128, description="Name for the new version") + publish_note: str = Field("", max_length=1024, description="Note for the new version") + + +class SkillPublishResponse(SkillVersionRow, Hinted): + pass + + +class SkillImportPayload(BaseModel): + model_config = ConfigDict(extra="forbid") + + files: UploadParts = Field(description="The skill folder, one part per file; SKILL.md at its root") + skill_id: UUIDStr | None = Field(None, description="Overwrite this skill's draft; omit to create a new skill") + draft_hash: str | None = Field( + None, + pattern=r"^[0-9]+$", + description="From describe.skill; rejects the overwrite if the draft changed since", + ) + + @field_validator("draft_hash") + @classmethod + def _needs_skill_id(cls, value: str | None, info: ValidationInfo) -> str | None: + if value is not None and info.data.get("skill_id") is None: + raise ValueError("draft_hash only applies to an overwrite; give skill_id too") + return value + + +class SkillImportResponse(Hinted): + id: str + name: str + draft_hash: str = Field(description="Opaque; names the draft this wrote") + file_count: int diff --git a/api/controllers/openapi/_multipart.py b/api/controllers/openapi/_multipart.py index fdd8ba07a0e..0cc51a400b6 100644 --- a/api/controllers/openapi/_multipart.py +++ b/api/controllers/openapi/_multipart.py @@ -21,12 +21,13 @@ from __future__ import annotations import json import re from collections.abc import Collection -from typing import Any, Final +from typing import Any, BinaryIO, Final, cast from flask import request from werkzeug.datastructures import FileStorage from controllers.openapi._errors import InvalidFilePart +from controllers.openapi._upload import UploadParts LIST_SUFFIX: Final = "[]" _PART_NAME_RE: Final = re.compile(r"^(?P[^\[\]]+)(?:\[(?P[^\[\]]+)\])?(?P\[\])?$") @@ -66,3 +67,12 @@ def body_from_request(*, file_fields: Collection[str] = ()) -> dict[str, Any]: for name in request.files: _place(body, name, request.files.getlist(name), file_fields=file_fields) return body + + +def folder_streams(parts: UploadParts) -> dict[str, BinaryIO]: + streams: dict[str, BinaryIO] = {} + for path, part in parts.items(): + if isinstance(part, list): + raise InvalidFilePart(f"{path}: given more than once") + streams[path] = cast(BinaryIO, part.stream) + return streams diff --git a/api/controllers/openapi/app_run.py b/api/controllers/openapi/app_run.py index 8182399cc96..dce07e94145 100644 --- a/api/controllers/openapi/app_run.py +++ b/api/controllers/openapi/app_run.py @@ -25,7 +25,7 @@ from controllers.common.rbac import PlainApp, RBACCheck, RBACPermission from controllers.openapi import openapi_ns from controllers.openapi._audit import emit_app_run from controllers.openapi._contract import Effect, Example, Kind, endpoint -from controllers.openapi._errors import AppModeMismatch, RunNotFound, RunNotStoppable +from controllers.openapi._errors import AppModeMismatch, RunNotFound, RunNotStoppable, require_uuid from controllers.openapi._files import end_read_transaction, materialize, merge_files from controllers.openapi._hints import attach_stream_hints from controllers.openapi._models import ( @@ -38,7 +38,6 @@ from controllers.openapi._models import ( TaskStopResponse, WorkflowRunPayload, ) -from controllers.openapi.app_workflow import require_uuid from controllers.openapi.auth.context import Context from controllers.openapi.auth.requirements import ( EDITOR_ROLES, diff --git a/api/controllers/openapi/app_workflow.py b/api/controllers/openapi/app_workflow.py index 2fbf8fce4f4..35d595a34c8 100644 --- a/api/controllers/openapi/app_workflow.py +++ b/api/controllers/openapi/app_workflow.py @@ -6,7 +6,6 @@ from __future__ import annotations from collections.abc import Iterable from http import HTTPStatus from typing import Any, Final -from uuid import UUID from flask_restx import Resource from werkzeug.exceptions import BadRequest @@ -21,12 +20,12 @@ from controllers.openapi._errors import ( DraftNotFound, EnvVariableNotFound, NodeNotFound, - OpenApiError, RunNotFound, SecretMaskNotSecret, SecretMaskUnknownId, VersionNotFound, VersionNotRestorable, + require_uuid, ) from controllers.openapi._files import end_read_transaction, merge_files from controllers.openapi._hints import cursor_after, next_page_hint, page_after @@ -89,15 +88,6 @@ _ENV_WRITE_GUARDS: Final = account_app_guards( ) -def require_uuid(value: str, *, not_found: type[OpenApiError]) -> str: - """Parse a path id as a UUID or raise 404; an unparsed id would otherwise reach - a UUID-typed database column and fail as a 500.""" - try: - return str(UUID(value)) - except ValueError as e: - raise not_found() from e - - def _require_run(ctx: Context, run_id: str) -> WorkflowRun: run = application_services().workflow_runs.get_workflow_run( ctx.request_context, app_id=ctx.app.id, run_id=require_uuid(run_id, not_found=RunNotFound) diff --git a/api/controllers/openapi/auth/requirements.py b/api/controllers/openapi/auth/requirements.py index 2caf4382af8..deaa8bd056e 100644 --- a/api/controllers/openapi/auth/requirements.py +++ b/api/controllers/openapi/auth/requirements.py @@ -27,6 +27,7 @@ from controllers.openapi._errors import ( MemberLimitExceeded, PluginInstallForbidden, ServiceApiDisabled, + SkillsDisabled, WebAppAccessRequiresEE, ) from controllers.openapi._models import Hint @@ -266,6 +267,17 @@ class CheckPluginInstallSetting(Requirement): raise PluginInstallForbidden(f"Plugin install setting {unknown!r} is not known to this server.") +class CheckSkillEnabled(Requirement): + """Ranked ahead of membership and permission checks so every caller gets 503 while Skills is off.""" + + rank = Rank.EARLY + + @override + def run(self, subject: Subject, ctx: Context, session: Session) -> None: + if not dify_config.ENABLE_SKILL: + raise SkillsDisabled() + + class CheckAppAccess(Requirement): """Run-scope comes from the declaration site, so it is not re-checked here. diff --git a/api/controllers/openapi/skills.py b/api/controllers/openapi/skills.py new file mode 100644 index 00000000000..bf5b5ac00b1 --- /dev/null +++ b/api/controllers/openapi/skills.py @@ -0,0 +1,449 @@ +"""Workspace skills: list, describe, export, import and publish.""" + +from __future__ import annotations + +from collections.abc import Callable, Generator, Mapping +from contextlib import contextmanager +from enum import StrEnum +from functools import partial +from http import HTTPStatus +from io import BytesIO +from types import MappingProxyType +from typing import Final + +from flask import send_file +from flask_restx import Resource + +from configs import dify_config +from constants import BYTES_PER_MIB +from controllers.common.fields import BinaryFileResponse +from controllers.common.rbac import RBACCheck, RBACPermission, Workspace +from controllers.openapi import openapi_ns +from controllers.openapi._contract import ( + CapRequestBody, + Example, + Kind, + endpoint, + multipart_header_allowance, + op_of, +) +from controllers.openapi._errors import ( + DraftChanged, + ErrorDetail, + OpenApiError, + SkillNameConflict, + SkillNameMismatch, + SkillNotFound, + SkillPackageInvalid, + SkillPackageTooLarge, + SkillRequestRejected, + SkillVersionNotFound, + require_uuid, +) +from controllers.openapi._models import ( + Hint, + SkillDescribeResponse, + SkillExportQuery, + SkillImportPayload, + SkillImportResponse, + SkillListQuery, + SkillListResponse, + SkillPublishPayload, + SkillPublishResponse, + SkillRow, + SkillVersionListQuery, + SkillVersionListResponse, + SkillVersionRow, +) +from controllers.openapi._multipart import folder_streams +from controllers.openapi._upload import UploadParts, file_fields +from controllers.openapi.auth.requirements import ( + EDITOR_ROLES, + WORKSPACE_READ_GUARDS, + CheckRBACPermission, + CheckSkillEnabled, + Requirement, + workspace_write_guards, +) +from libs.archive_writer import ( + ArchiveEmptyError, + ArchiveError, + ArchiveLimitKind, + ArchiveLimits, + ArchiveOptions, + ArchivePathError, + ArchiveTooLargeError, + StrictPathPolicy, + entries_from_folder, + write_archive, +) +from machinery.context import RequestContext +from services.skill_management_service import ( + SkillErrorDetail, + SkillManagementService, + SkillManagementServiceError, + SkillOverwritePayload, + SkillServiceErrorCode, + max_skill_archive_bytes, +) +from services.skill_management_service import SkillImportPayload as ServiceSkillImportPayload +from services.skill_management_service import SkillPublishPayload as ServiceSkillPublishPayload + + +class ServiceRejection(StrEnum): + """Service error codes this surface answers with its own error; any other code is relayed.""" + + error: type[OpenApiError] + + NOT_FOUND = (SkillServiceErrorCode.SKILL_NOT_FOUND, SkillNotFound) + VERSION_NOT_FOUND = (SkillServiceErrorCode.SKILL_VERSION_NOT_FOUND, SkillVersionNotFound) + CONFLICT = (SkillServiceErrorCode.SKILL_CONFLICT, DraftChanged) + NAME_CONFLICT = (SkillServiceErrorCode.SKILL_NAME_CONFLICT, SkillNameConflict) + NAME_MISMATCH = (SkillServiceErrorCode.SKILL_NAME_MISMATCH, SkillNameMismatch) + MISSING_SKILL_MD = (SkillServiceErrorCode.MISSING_SKILL_MD, SkillPackageInvalid) + INVALID_SKILL_MD = (SkillServiceErrorCode.INVALID_SKILL_MD, SkillPackageInvalid) + MISSING_SKILL_NAME = (SkillServiceErrorCode.MISSING_SKILL_NAME, SkillPackageInvalid) + INVALID_SKILL_NAME = (SkillServiceErrorCode.INVALID_SKILL_NAME, SkillPackageInvalid) + MISSING_SKILL_DESCRIPTION = (SkillServiceErrorCode.MISSING_SKILL_DESCRIPTION, SkillPackageInvalid) + INVALID_SKILL_DESCRIPTION = (SkillServiceErrorCode.INVALID_SKILL_DESCRIPTION, SkillPackageInvalid) + INVALID_SKILL_PACKAGE = (SkillServiceErrorCode.INVALID_SKILL_PACKAGE, SkillPackageInvalid) + DUPLICATE_FILE_PATH = (SkillServiceErrorCode.DUPLICATE_FILE_PATH, SkillPackageInvalid) + FILE_PATH_CONFLICT = (SkillServiceErrorCode.FILE_PATH_CONFLICT, SkillPackageInvalid) + INVALID_SKILL_FILE = (SkillServiceErrorCode.INVALID_SKILL_FILE, SkillPackageInvalid) + ARCHIVE_TOO_LARGE = (SkillServiceErrorCode.ARCHIVE_TOO_LARGE, SkillPackageTooLarge) + SKILL_TOO_LARGE = (SkillServiceErrorCode.SKILL_TOO_LARGE, SkillPackageTooLarge) + TOO_MANY_FILES = (SkillServiceErrorCode.TOO_MANY_FILES, SkillPackageTooLarge) + + def __new__(cls, value: str, error: type[OpenApiError]) -> ServiceRejection: + member = str.__new__(cls, value) + member._value_ = value + member.error = error + return member + + +type HintsFor = Callable[[SkillManagementServiceError], list[Hint]] + +_NO_HINTS: Final[Mapping[type[OpenApiError], HintsFor]] = MappingProxyType({}) + +_IMPORT_FILENAME: Final = "skill.zip" + +_LIMIT_MESSAGES: Final[Mapping[ArchiveLimitKind, Callable[[int], str]]] = { + ArchiveLimitKind.ENTRIES: lambda limit: f"The skill folder has more than {limit} files", + ArchiveLimitKind.BYTES: lambda limit: f"The skill folder is over {limit // BYTES_PER_MIB} MiB", +} + +_READ_GUARDS: Final = ( + CheckSkillEnabled(), + *WORKSPACE_READ_GUARDS, + CheckRBACPermission(RBACCheck(RBACPermission.SKILL_VIEW, Workspace())), +) + + +def _edit_guards(permission: RBACPermission) -> tuple[Requirement, ...]: + return (CheckSkillEnabled(), *workspace_write_guards(permission, roles=EDITOR_ROLES)) + + +def _details(error: SkillManagementServiceError) -> list[ErrorDetail]: + return [ + ErrorDetail(type=error.code, msg=error.message), + *(ErrorDetail(type=error.code, loc=[key], msg=str(value)) for key, value in error.details.items()), + ] + + +def _rejection(error: SkillManagementServiceError) -> OpenApiError: + try: + known = ServiceRejection(error.code) + except ValueError: + return SkillRequestRejected(error.message, status_code=error.status_code, details=_details(error)) + return known.error(error.message, details=_details(error)) + + +@contextmanager +def _relaying_service_errors(*, hints: Mapping[type[OpenApiError], HintsFor] = _NO_HINTS) -> Generator[None]: + try: + yield + except SkillManagementServiceError as error: + rejection = _rejection(error) + hints_for = hints.get(type(rejection)) + if hints_for is not None: + rejection.hints = hints_for(error) + raise rejection from error + + +def _name_conflict_hints(error: SkillManagementServiceError, *, workspace_id: str) -> list[Hint]: + skill_id = error.details.get(SkillErrorDetail.SKILL_ID) + if skill_id is not None: + return [ + Hint( + summary="Overwrite the existing skill instead", + op=op_of(SkillImportApi.post), + input={"workspace_id": workspace_id, "skill_id": skill_id, "files": None}, + ) + ] + name = error.details.get(SkillErrorDetail.NAME) + if name is not None: + return [ + Hint( + summary="Find the skill that has this name", + op=op_of(SkillListApi.get), + input={"workspace_id": workspace_id, "keyword": name}, + ) + ] + return [] + + +def _skill_upload_parts() -> int: + plain_fields = len(SkillImportPayload.model_fields) - len(file_fields(SkillImportPayload)) + return dify_config.SKILL_PACKAGE_MAX_ENTRIES + plain_fields + + +_UPLOAD_CAP: Final = CapRequestBody( + max_bytes=lambda: max_skill_archive_bytes() + multipart_header_allowance(_skill_upload_parts()), + max_parts=_skill_upload_parts, +) + + +def _archive_rejection(error: ArchiveError) -> OpenApiError: + if isinstance(error, ArchiveTooLargeError): + return SkillPackageTooLarge(_LIMIT_MESSAGES[error.kind](error.limit)) + if isinstance(error, ArchivePathError): + detail = ErrorDetail(type=error.reason.value, loc=[error.path], msg=error.reason.message) + return SkillPackageInvalid(str(error), details=[detail]) + if isinstance(error, ArchiveEmptyError): + return SkillPackageInvalid("The skill folder has no files") + raise error + + +def _archive_of(folder: UploadParts) -> bytes: + options = ArchiveOptions( + StrictPathPolicy(), + ArchiveLimits(max_entries=dify_config.SKILL_PACKAGE_MAX_ENTRIES, max_bytes=max_skill_archive_bytes()), + ) + try: + result = write_archive(entries_from_folder(folder_streams(folder)), options) + except ArchiveError as error: + raise _archive_rejection(error) from error + try: + return result.file.read() + finally: + result.file.close() + + +def _draft_hash(updated_at_epoch: int) -> str: + return str(updated_at_epoch) + + +def _expected_updated_at(draft_hash: str | None) -> int | None: + if draft_hash is None: + return None + return int(draft_hash) + + +@openapi_ns.route("/workspaces//skills") +class SkillListApi(Resource): + @endpoint( + context="workspace", + op="get.skill", + kind=Kind.LIST, + summary="Skills in the workspace, newest edit first", + examples=( + Example(title="List skills", input={}), + Example(title="Find skills by words and tag", input={"keyword": "expense", "tag": ["ops"]}), + ), + requirements=_READ_GUARDS, + query=SkillListQuery, + returns=(HTTPStatus.OK, SkillListResponse, "Skills"), + ) + def get(self, ctx: RequestContext, workspace_id: str, *, query: SkillListQuery): + with _relaying_service_errors(): + result = SkillManagementService().list_skills( + tenant_id=ctx.active_workspace_id, + keyword=query.keyword, + tags=query.tag, + page=query.page, + limit=query.limit, + ) + return SkillListResponse.build( + page=query.page, + limit=query.limit, + total=result["total"], + items=[SkillRow.model_validate(record) for record in result["data"]], + ) + + +@openapi_ns.route("/workspaces//skills/") +class SkillDescribeApi(Resource): + @endpoint( + context="workspace", + op="describe.skill", + kind=Kind.OBJECT, + summary="One skill with its draft hash and latest published version", + examples=(Example(title="Describe a skill", input={"skill_id": ""}),), + requirements=_READ_GUARDS, + returns=(HTTPStatus.OK, SkillDescribeResponse, "Skill"), + ) + def get(self, ctx: RequestContext, workspace_id: str, skill_id: str): + skill_id = require_uuid(skill_id, not_found=SkillNotFound) + service = SkillManagementService() + with _relaying_service_errors(): + record = service.get_skill(tenant_id=ctx.active_workspace_id, skill_id=skill_id) + versions = service.list_versions(tenant_id=ctx.active_workspace_id, skill_id=skill_id)["data"] + latest = next((version for version in versions if version["is_latest"]), None) + return SkillDescribeResponse.model_validate( + { + **record, + "draft_hash": _draft_hash(record["updated_at"]), + "latest_version": SkillVersionRow.model_validate(latest) if latest else None, + } + ) + + +@openapi_ns.route("/workspaces//skills//versions") +class SkillVersionListApi(Resource): + @endpoint( + context="workspace", + op="get.skill.version", + kind=Kind.LIST, + summary="Published versions of a skill, newest first", + examples=(Example(title="List versions of a skill", input={"skill_id": ""}),), + requirements=_READ_GUARDS, + query=SkillVersionListQuery, + returns=(HTTPStatus.OK, SkillVersionListResponse, "Versions"), + ) + def get(self, ctx: RequestContext, workspace_id: str, skill_id: str, *, query: SkillVersionListQuery): + skill_id = require_uuid(skill_id, not_found=SkillNotFound) + with _relaying_service_errors(): + records = SkillManagementService().list_versions(tenant_id=ctx.active_workspace_id, skill_id=skill_id) + rows = [SkillVersionRow.model_validate(record) for record in records["data"]] + return SkillVersionListResponse.page_of(rows, query=query) + + +@openapi_ns.route("/workspaces//skills//archive") +class SkillExportApi(Resource): + @endpoint( + context="workspace", + op="export.skill", + kind=Kind.FILE, + summary="Download a skill as a zip: its draft, or one published version", + examples=( + Example(title="Export the draft", input={"skill_id": ""}), + Example(title="Export a published version", input={"skill_id": "", "version_id": ""}), + ), + requirements=_READ_GUARDS, + query=SkillExportQuery, + returns=(HTTPStatus.OK, BinaryFileResponse, "Zip archive"), + ) + def get(self, ctx: RequestContext, workspace_id: str, skill_id: str, *, query: SkillExportQuery): + skill_id = require_uuid(skill_id, not_found=SkillNotFound) + service = SkillManagementService() + with _relaying_service_errors(): + if query.version_id is None: + archive = service.export_draft_archive(tenant_id=ctx.active_workspace_id, skill_id=skill_id) + else: + archive = service.export_version_archive( + tenant_id=ctx.active_workspace_id, skill_id=skill_id, version_id=query.version_id + ) + return send_file( + BytesIO(archive.payload), mimetype=archive.mime_type, as_attachment=True, download_name=archive.filename + ) + + +@openapi_ns.route("/workspaces//skills/:publish") +class SkillPublishApi(Resource): + @endpoint( + context="workspace", + op="publish.skill", + kind=Kind.OBJECT, + summary="Publish the draft as a new version", + examples=( + Example(title="Publish the draft", input={"skill_id": ""}), + Example( + title="Publish a named version", + input={"skill_id": "", "version_name": "v2", "publish_note": "Adds a retry rule"}, + ), + ), + requirements=_edit_guards(RBACPermission.SKILL_PUBLISH), + body=SkillPublishPayload, + returns=(HTTPStatus.OK, SkillPublishResponse, "Published"), + ) + def post(self, ctx: RequestContext, workspace_id: str, skill_id: str, *, body: SkillPublishPayload): + skill_id = require_uuid(skill_id, not_found=SkillNotFound) + with _relaying_service_errors(): + record = SkillManagementService().publish_skill( + tenant_id=ctx.active_workspace_id, + user_id=ctx.account_id, + skill_id=skill_id, + payload=ServiceSkillPublishPayload(version_name=body.version_name, publish_note=body.publish_note), + ) + return SkillPublishResponse.model_validate(record) + + +@openapi_ns.route("/workspaces//skills/imports") +class SkillImportApi(Resource): + @endpoint( + context="workspace", + op="import.skill", + kind=Kind.OBJECT, + summary="Create a skill from a folder, or overwrite a skill's draft with one", + examples=( + Example(title="Create a skill", input={"files": {"SKILL.md": "./my-skill/SKILL.md"}}), + Example( + title="Overwrite a skill's draft", + input={ + "files": {"SKILL.md": "./my-skill/SKILL.md"}, + "skill_id": "", + "draft_hash": "1761200000", + }, + ), + ), + requirements=(*_edit_guards(RBACPermission.SKILL_EDIT), _UPLOAD_CAP), + body=SkillImportPayload, + returns=( + (HTTPStatus.CREATED, SkillImportResponse, "Created"), + (HTTPStatus.OK, SkillImportResponse, "Overwritten"), + ), + ) + def post(self, ctx: RequestContext, workspace_id: str, *, body: SkillImportPayload): + """Import never publishes. ``draft_hash`` is the draft's ``updated_at`` in whole seconds, + so an edit made in the same second as the export is not detected. + """ + content = _archive_of(body.files) + workspace = ctx.active_workspace_id + name_conflict_hints: Mapping[type[OpenApiError], HintsFor] = { + SkillNameConflict: partial(_name_conflict_hints, workspace_id=workspace) + } + service = SkillManagementService() + with _relaying_service_errors(hints=name_conflict_hints): + if body.skill_id is None: + status = HTTPStatus.CREATED + record = service.import_skill( + tenant_id=workspace, + user_id=ctx.account_id, + payload=ServiceSkillImportPayload(content=content, filename=_IMPORT_FILENAME), + ) + else: + status = HTTPStatus.OK + record = service.overwrite_skill( + tenant_id=workspace, + user_id=ctx.account_id, + skill_id=body.skill_id, + payload=SkillOverwritePayload( + content=content, expected_updated_at=_expected_updated_at(body.draft_hash) + ), + ) + return ( + SkillImportResponse( + id=record["id"], + name=record["name"], + draft_hash=_draft_hash(record["updated_at"]), + file_count=len(record["files"]), + hints=[ + Hint( + summary="Publish so agents use it", + op=op_of(SkillPublishApi.post), + input={"workspace_id": workspace, "skill_id": record["id"]}, + ) + ], + ), + status, + ) diff --git a/api/controllers/openapi/workflow_events.py b/api/controllers/openapi/workflow_events.py index d7e6e7e5a7b..6b35be8a9fd 100644 --- a/api/controllers/openapi/workflow_events.py +++ b/api/controllers/openapi/workflow_events.py @@ -20,8 +20,7 @@ from controllers.common.fields import EventStreamResponse from controllers.common.rbac import PlainApp, RBACCheck, RBACPermission from controllers.openapi import openapi_ns from controllers.openapi._contract import Example, Kind, endpoint -from controllers.openapi._errors import RunNotFound -from controllers.openapi.app_workflow import require_uuid +from controllers.openapi._errors import RunNotFound, require_uuid from controllers.openapi.auth.context import Context from controllers.openapi.auth.requirements import ( CheckAppAccess, diff --git a/api/openapi/markdown/openapi-openapi.md b/api/openapi/markdown/openapi-openapi.md index 73cd80c103c..b64969e6150 100644 --- a/api/openapi/markdown/openapi-openapi.md +++ b/api/openapi/markdown/openapi-openapi.md @@ -1982,6 +1982,124 @@ Returns a depth- and page-size-bounded tree rooted at path, equivalent to difyct | 422 | Validation error | **application/json**: [ErrorBody](#errorbody)
| | default | Error | **application/json**: [ErrorBody](#errorbody)
| +### [GET] /workspaces/{workspace_id}/skills +#### Parameters + +| Name | Located in | Description | Required | Schema | +| ---- | ---------- | ----------- | -------- | ------ | +| keyword | query | Words to match in the name, display name or description | No | string | +| limit | query | | No | integer,
**Default:** 20 | +| page | query | | No | integer,
**Default:** 1 | +| tag | query | Only skills with every one of these tags; repeat to give several | No | [ string ] | +| workspace_id | path | | Yes | string | + +#### Responses + +| Code | Description | Schema | +| ---- | ----------- | ------ | +| 200 | Skills | **application/json**: [SkillListResponse](#skilllistresponse)
| +| 422 | Validation error | **application/json**: [ErrorBody](#errorbody)
| +| default | Error | **application/json**: [ErrorBody](#errorbody)
| + +### [POST] /workspaces/{workspace_id}/skills/imports +**Import never publishes** + +``draft_hash`` is the draft's ``updated_at`` in whole seconds, +so an edit made in the same second as the export is not detected. + +#### Parameters + +| Name | Located in | Description | Required | Schema | +| ---- | ---------- | ----------- | -------- | ------ | +| workspace_id | path | | Yes | string | + +#### Request Body + +| Required | Schema | +| -------- | ------ | +| Yes | **application/json**: [SkillImportPayload](#skillimportpayload)
**multipart/form-data**: [SkillImportPayload](#skillimportpayload)
| + +#### Responses + +| Code | Description | Schema | +| ---- | ----------- | ------ | +| 200 | Overwritten | **application/json**: [SkillImportResponse](#skillimportresponse)
| +| 201 | Created | **application/json**: [SkillImportResponse](#skillimportresponse)
| +| 422 | Validation error | **application/json**: [ErrorBody](#errorbody)
| +| default | Error | **application/json**: [ErrorBody](#errorbody)
| + +### [GET] /workspaces/{workspace_id}/skills/{skill_id} +#### Parameters + +| Name | Located in | Description | Required | Schema | +| ---- | ---------- | ----------- | -------- | ------ | +| skill_id | path | | Yes | string | +| workspace_id | path | | Yes | string | + +#### Responses + +| Code | Description | Schema | +| ---- | ----------- | ------ | +| 200 | Skill | **application/json**: [SkillDescribeResponse](#skilldescriberesponse)
| +| default | Error | **application/json**: [ErrorBody](#errorbody)
| + +### [GET] /workspaces/{workspace_id}/skills/{skill_id}/archive +#### Parameters + +| Name | Located in | Description | Required | Schema | +| ---- | ---------- | ----------- | -------- | ------ | +| version_id | query | Export this published version instead of the draft | No | string (uuid) | +| skill_id | path | | Yes | string | +| workspace_id | path | | Yes | string | + +#### Responses + +| Code | Description | Schema | +| ---- | ----------- | ------ | +| 200 | Zip archive | **application/json**: [BinaryFileResponse](#binaryfileresponse)
| +| 422 | Validation error | **application/json**: [ErrorBody](#errorbody)
| +| default | Error | **application/json**: [ErrorBody](#errorbody)
| + +### [GET] /workspaces/{workspace_id}/skills/{skill_id}/versions +#### Parameters + +| Name | Located in | Description | Required | Schema | +| ---- | ---------- | ----------- | -------- | ------ | +| limit | query | | No | integer,
**Default:** 20 | +| page | query | | No | integer,
**Default:** 1 | +| skill_id | path | | Yes | string | +| workspace_id | path | | Yes | string | + +#### Responses + +| Code | Description | Schema | +| ---- | ----------- | ------ | +| 200 | Versions | **application/json**: [SkillVersionListResponse](#skillversionlistresponse)
| +| 422 | Validation error | **application/json**: [ErrorBody](#errorbody)
| +| default | Error | **application/json**: [ErrorBody](#errorbody)
| + +### [POST] /workspaces/{workspace_id}/skills/{skill_id}:publish +#### Parameters + +| Name | Located in | Description | Required | Schema | +| ---- | ---------- | ----------- | -------- | ------ | +| skill_id | path | | Yes | string | +| workspace_id | path | | Yes | string | + +#### Request Body + +| Required | Schema | +| -------- | ------ | +| Yes | **application/json**: [SkillPublishPayload](#skillpublishpayload)
| + +#### Responses + +| Code | Description | Schema | +| ---- | ----------- | ------ | +| 200 | Published | **application/json**: [SkillPublishResponse](#skillpublishresponse)
| +| 422 | Validation error | **application/json**: [ErrorBody](#errorbody)
| +| default | Error | **application/json**: [ErrorBody](#errorbody)
| + ### [GET] /workspaces/{workspace_id}/tool-providers/{provider} #### Parameters @@ -2364,6 +2482,12 @@ mode is a closed enum of listable app types. | max_active_requests | integer | | No | | name | string | | Yes | +#### BinaryFileResponse + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| BinaryFileResponse | string | | | + #### ChatAppInfo | Name | Type | Description | Required | @@ -3649,6 +3773,127 @@ Pagination for GET /account/sessions. Strict (extra='forbid'). | ---- | ---- | ----------- | -------- | | result | string | Operation result. | Yes | +#### SkillDescribeResponse + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| description | string | | Yes | +| display_name | string | | Yes | +| draft_hash | string | Opaque; names the draft this was read from | Yes | +| hints | [ [Hint](#hint) ] | Next steps the caller can take | No | +| icon | string | | Yes | +| id | string | | Yes | +| latest_published_version_id | string | Null while the skill has never been published | Yes | +| latest_version | [SkillVersionRow](#skillversionrow) | Null while the skill has never been published | Yes | +| name | string | | Yes | +| tags | [ string ] | | Yes | +| updated_at | integer | | Yes | + +#### SkillExportQuery + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| version_id | string | Export this published version instead of the draft | No | + +#### SkillImportPayload + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| draft_hash | string | From describe.skill; rejects the overwrite if the draft changed since | No | +| files | object | The skill folder, one part per file; SKILL.md at its root | Yes | +| skill_id | string | Overwrite this skill's draft; omit to create a new skill | No | + +#### SkillImportResponse + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| draft_hash | string | Opaque; names the draft this wrote | Yes | +| file_count | integer | | Yes | +| hints | [ [Hint](#hint) ] | Next steps the caller can take | No | +| id | string | | Yes | +| name | string | | Yes | + +#### SkillListQuery + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| keyword | string | Words to match in the name, display name or description | No | +| limit | integer,
**Default:** 20 | | No | +| page | integer,
**Default:** 1 | | No | +| tag | [ string ] | Only skills with every one of these tags; repeat to give several | No | + +#### SkillListResponse + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| data | [ [SkillRow](#skillrow) ] | | Yes | +| has_more | boolean | | Yes | +| hints | [ [Hint](#hint) ] | Next steps the caller can take | No | +| limit | integer | | Yes | +| page | integer | | Yes | +| total | integer | | Yes | + +#### SkillPublishPayload + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| publish_note | string | Note for the new version | No | +| version_name | string | Name for the new version | No | + +#### SkillPublishResponse + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| created_at | integer | | Yes | +| created_by | string | Account id that published it | Yes | +| hints | [ [Hint](#hint) ] | Next steps the caller can take | No | +| id | string | | Yes | +| publish_note | string | | Yes | +| version_name | string | | Yes | +| version_number | integer | | Yes | + +#### SkillRow + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| description | string | | Yes | +| display_name | string | | Yes | +| icon | string | | Yes | +| id | string | | Yes | +| latest_published_version_id | string | Null while the skill has never been published | Yes | +| name | string | | Yes | +| tags | [ string ] | | Yes | +| updated_at | integer | | Yes | + +#### SkillVersionListQuery + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| limit | integer,
**Default:** 20 | | No | +| page | integer,
**Default:** 1 | | No | + +#### SkillVersionListResponse + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| data | [ [SkillVersionRow](#skillversionrow) ] | | Yes | +| has_more | boolean | | Yes | +| hints | [ [Hint](#hint) ] | Next steps the caller can take | No | +| limit | integer | | Yes | +| page | integer | | Yes | +| total | integer | | Yes | + +#### SkillVersionRow + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| created_at | integer | | Yes | +| created_by | string | Account id that published it | Yes | +| id | string | | Yes | +| publish_note | string | | Yes | +| version_name | string | | Yes | +| version_number | integer | | Yes | + #### SubjectType | Name | Type | Description | Required | diff --git a/api/services/skill_management_service.py b/api/services/skill_management_service.py index b6a2b59c6d9..f3f0f358f05 100644 --- a/api/services/skill_management_service.py +++ b/api/services/skill_management_service.py @@ -21,9 +21,9 @@ import re import xml.etree.ElementTree as ET import zipfile from base64 import b64encode -from collections.abc import Generator +from collections.abc import Generator, Mapping from contextlib import contextmanager, suppress -from dataclasses import dataclass +from dataclasses import dataclass, field from datetime import datetime from enum import StrEnum from typing import Any, Literal @@ -39,6 +39,7 @@ from sqlalchemy.orm import Session, object_session from yaml.error import MarkedYAMLError from configs import dify_config +from constants import BYTES_PER_MIB from core.credit_usage import CreditUsageCreatedBy from core.db.session_factory import session_factory from core.errors.error import ProviderTokenNotInitError @@ -55,6 +56,7 @@ from graphon.model_runtime.entities.message_entities import ( from graphon.model_runtime.entities.model_entities import ModelFeature, ModelType from graphon.model_runtime.errors.invoke import InvokeError from graphon.nodes.llm.reasoning import split_reasoning +from libs.archive_writer import is_os_junk from libs.datetime_utils import naive_utc_now from libs.helper import escape_like_pattern from models.account import Account @@ -99,6 +101,7 @@ from services.file_service import FileService logger = logging.getLogger(__name__) _SKILL_MD = "SKILL.md" +_ZIP_MIME_TYPE = "application/zip" _MAX_FILE_CHECK_ITEMS = 100 _MAX_SKILLS_PER_WORKSPACE = 500 _MAX_AGENT_SKILLS = 20 @@ -235,6 +238,34 @@ _MAX_ASSISTANT_PDF_PAGES = 100 _SKILL_ASSISTANT_ROLE = "__skill_authoring_assistant__" +class SkillServiceErrorCode(StrEnum): + SKILL_NOT_FOUND = "skill_not_found" + SKILL_VERSION_NOT_FOUND = "skill_version_not_found" + SKILL_CONFLICT = "skill_conflict" + SKILL_NAME_CONFLICT = "skill_name_conflict" + SKILL_NAME_MISMATCH = "skill_name_mismatch" + MISSING_SKILL_MD = "missing_skill_md" + INVALID_SKILL_MD = "invalid_skill_md" + MISSING_SKILL_NAME = "missing_skill_name" + INVALID_SKILL_NAME = "invalid_skill_name" + MISSING_SKILL_DESCRIPTION = "missing_skill_description" + INVALID_SKILL_DESCRIPTION = "invalid_skill_description" + INVALID_SKILL_PACKAGE = "invalid_skill_package" + ARCHIVE_TOO_LARGE = "archive_too_large" + SKILL_TOO_LARGE = "skill_too_large" + TOO_MANY_FILES = "too_many_files" + DUPLICATE_FILE_PATH = "duplicate_file_path" + FILE_PATH_CONFLICT = "file_path_conflict" + INVALID_SKILL_FILE = "invalid_skill_file" + + +class SkillErrorDetail(StrEnum): + """Keys of ``SkillManagementServiceError.details`` that callers read back.""" + + SKILL_ID = "skill_id" + NAME = "name" + + class SkillManagementServiceError(Exception): """Skill operation failure mapped to HTTP status at controller boundaries.""" @@ -402,6 +433,13 @@ class SkillImportPayload(BaseModel): filename: str +class SkillOverwritePayload(BaseModel): + model_config = ConfigDict(arbitrary_types_allowed=True, extra="forbid") + + content: bytes + expected_updated_at: int | None = None + + class SkillRestorePayload(BaseModel): model_config = ConfigDict(extra="forbid") @@ -528,6 +566,43 @@ class RuntimeAgentSkillArchive: hash: str +@dataclass(frozen=True, slots=True) +class _BinaryArchiveEntry: + path: str + payload: bytes + + +@dataclass(frozen=True, slots=True) +class _SkillArchiveContents: + """Directories and text files are final draft items; binaries still need a stored tool file.""" + + entries: tuple[SkillDraftTreeItemPayload | _BinaryArchiveEntry, ...] + metadata: dict[str, Any] + skill_md_content: str + + +@dataclass(frozen=True, slots=True) +class _SkillPackage: + contents: _SkillArchiveContents + name: str + description: str + + +@dataclass(slots=True) +class _ToolFileUploads: + """Tool files one request stores or reuses. + + ``candidate_ids`` maps a draft row's claimed ``(sha256, size)`` to its tool file ids. The claim is + client-set, so ``stored_digests`` caches the sha256 of each candidate's real bytes (None if unreadable). + """ + + tenant_id: str + user_id: str + candidate_ids: Mapping[tuple[str, int], tuple[str, ...]] = field(default_factory=dict) + stored_digests: dict[str, str | None] = field(default_factory=dict) + created: list[ToolFile] = field(default_factory=list) + + @dataclass(frozen=True, slots=True) class SkillFileContent: filename: str @@ -539,6 +614,30 @@ class SkillFileContent: hash: str +def max_skill_archive_bytes() -> int: + return dify_config.UPLOAD_SKILL_FILE_SIZE_LIMIT * BYTES_PER_MIB + + +def _size_text(size_bytes: int) -> str: + if size_bytes % BYTES_PER_MIB: + return f"{size_bytes} bytes" + return f"{size_bytes // BYTES_PER_MIB} MiB" + + +def _too_many_files_error() -> SkillManagementServiceError: + return SkillManagementServiceError( + SkillServiceErrorCode.TOO_MANY_FILES, + f"skill has more than {dify_config.SKILL_PACKAGE_MAX_ENTRIES} files", + ) + + +def _skill_too_large_error() -> SkillManagementServiceError: + return SkillManagementServiceError( + SkillServiceErrorCode.SKILL_TOO_LARGE, + f"skill is over {_size_text(dify_config.SKILL_PACKAGE_MAX_UNCOMPRESSED_BYTES)} uncompressed", + ) + + def validate_skill_name(name: str) -> str: """Validate the PRD Skill name using the existing config-skill name baseline.""" normalized = validate_config_skill_name(name) @@ -1739,26 +1838,69 @@ class SkillManagementService: skill = self._require_skill(session, tenant_id=tenant_id, skill_id=skill_id) self._check_expected_updated_at(skill, payload.expected_updated_at) files = self._build_draft_rows_from_tree(skill=skill, payload=payload, strict_frontmatter=False) - session.execute(delete(SkillDraftFile).where(SkillDraftFile.skill_id == skill.id)) - session.flush() - for file in files: - session.add(file) - skill.updated_by = user_id - skill.updated_at = naive_utc_now() - session.flush() - try: - session.commit() - except IntegrityError as exc: - session.rollback() - raise SkillManagementServiceError("skill_name_conflict", "skill name already exists") from exc - return { - **self._serialize_skill( - skill, - tags=self._skill_tags_by_id(session, tenant_id=tenant_id, skill_ids=[skill.id]).get(skill.id, []), - accounts=self._skill_accounts(session, skill=skill), - ), - "files": [self._serialize_file(file) for file in sorted(files, key=lambda item: item.path)], - } + self._replace_draft_rows(session, skill=skill, files=files, user_id=user_id) + return self._serialize_skill_with_files(session, tenant_id=tenant_id, skill=skill, files=files) + + def overwrite_skill( + self, + *, + tenant_id: str, + user_id: str, + skill_id: str, + payload: SkillOverwritePayload, + ) -> dict[str, Any]: + with self._session_scope() as session: + skill = self._require_skill(session, tenant_id=tenant_id, skill_id=skill_id) + self._check_expected_updated_at(skill, payload.expected_updated_at) + package = self._read_skill_package(payload.content) + if package.name != skill.name: + raise SkillManagementServiceError( + SkillServiceErrorCode.SKILL_NAME_MISMATCH, + "skill package name does not match the existing skill", + details={"expected": skill.name, "got": package.name}, + ) + files = self._replace_draft_from_archive( + session, + skill=skill, + user_id=user_id, + contents=package.contents, + display_name=self._display_name_from_frontmatter(metadata=package.contents.metadata, name=package.name), + ) + return self._serialize_skill_with_files(session, tenant_id=tenant_id, skill=skill, files=files) + + def _serialize_skill_with_files( + self, + session, + *, + tenant_id: str, + skill: Skill, + files: list[SkillDraftFile], + ) -> dict[str, Any]: + return { + **self._serialize_skill( + skill, + tags=self._skill_tags_by_id(session, tenant_id=tenant_id, skill_ids=[skill.id]).get(skill.id, []), + accounts=self._skill_accounts(session, skill=skill), + ), + "files": [self._serialize_file(file) for file in sorted(files, key=lambda item: item.path)], + } + + @staticmethod + def _replace_draft_rows(session, *, skill: Skill, files: list[SkillDraftFile], user_id: str) -> None: + session.execute(delete(SkillDraftFile).where(SkillDraftFile.skill_id == skill.id)) + session.flush() + for file in files: + session.add(file) + skill.updated_by = user_id + skill.updated_at = naive_utc_now() + session.flush() + try: + session.commit() + except IntegrityError as exc: + session.rollback() + raise SkillManagementServiceError( + SkillServiceErrorCode.SKILL_NAME_CONFLICT, "skill name already exists" + ) from exc def apply_draft_file_operation( self, @@ -1878,7 +2020,7 @@ class SkillManagementService: tenant_id=tenant_id, conversation_id=None, file_binary=archive_bytes, - mimetype="application/zip", + mimetype=_ZIP_MIME_TYPE, filename=f"{skill_name}.zip", ) with self._session_scope() as session: @@ -2123,40 +2265,41 @@ class SkillManagementService: } def import_skill(self, *, tenant_id: str, user_id: str, payload: SkillImportPayload) -> dict[str, Any]: - max_archive_bytes = dify_config.UPLOAD_SKILL_FILE_SIZE_LIMIT * 1024 * 1024 - if len(payload.content) > max_archive_bytes: - raise SkillManagementServiceError("archive_too_large", "skill archive exceeds size limit") - - draft_payload, metadata, skill_md_content = self._draft_payload_from_zip( - tenant_id=tenant_id, - user_id=user_id, - archive_bytes=payload.content, - ) - name = validate_skill_name(str(metadata.get("name") or "")) - description = self._require_frontmatter_description(metadata, content=skill_md_content) - display_name = self._display_name_from_frontmatter(metadata=metadata, name=name) + package = self._read_skill_package(payload.content) + display_name = self._display_name_from_frontmatter(metadata=package.contents.metadata, name=package.name) with self._session_scope() as session: self._enforce_workspace_skill_limit(session, tenant_id=tenant_id) - skill = Skill( - tenant_id=tenant_id, - name=name, - display_name=display_name, - icon="📄", - description=description[:1024], - name_manually_edited=True, - created_by=user_id, - updated_by=user_id, - ) - session.add(skill) - try: - session.flush() - files = self._build_draft_rows_from_tree(skill=skill, payload=draft_payload) - for file in files: - session.add(file) - session.commit() - except IntegrityError as exc: - session.rollback() - raise SkillManagementServiceError("skill_name_conflict", "skill name already exists") from exc + self._reject_taken_skill_name(session, tenant_id=tenant_id, name=package.name) + # Ends the pre-check read transaction so none stays open while the binaries upload. + session.commit() + uploads = _ToolFileUploads(tenant_id=tenant_id, user_id=user_id) + with self._discard_uploads_on_failure(session, uploads=uploads): + draft_payload = self._store_archive_entries(package.contents, uploads=uploads) + skill = Skill( + tenant_id=tenant_id, + name=package.name, + display_name=display_name, + icon="📄", + description=package.description[:1024], + name_manually_edited=True, + created_by=user_id, + updated_by=user_id, + ) + session.add(skill) + try: + session.flush() + files = self._build_draft_rows_from_tree(skill=skill, payload=draft_payload) + for file in files: + session.add(file) + session.commit() + except IntegrityError as exc: + session.rollback() + # The name was free at the pre-check, so which skill took it since is not known here. + raise SkillManagementServiceError( + SkillServiceErrorCode.SKILL_NAME_CONFLICT, + "skill name already exists", + details={SkillErrorDetail.NAME: package.name}, + ) from exc session.refresh(skill) return { **self._serialize_skill(skill, tags=[], accounts=self._skill_accounts(session, skill=skill)), @@ -2206,19 +2349,8 @@ class SkillManagementService: archive_bytes = self._load_tool_file_bytes(tenant_id=tenant_id, file_id=archive_file_id) with self._session_scope() as session: skill = self._require_skill(session, tenant_id=tenant_id, skill_id=skill_id) - draft_files = self._draft_rows_from_archive_bytes( - tenant_id=tenant_id, - user_id=user_id, - skill=skill, - archive_bytes=archive_bytes, - ) - session.execute(delete(SkillDraftFile).where(SkillDraftFile.skill_id == skill.id)) - session.flush() - for file in draft_files: - session.add(file) - skill.updated_by = user_id - skill.updated_at = naive_utc_now() - session.commit() + contents = self._read_skill_archive(archive_bytes=archive_bytes) + self._replace_draft_from_archive(session, skill=skill, user_id=user_id, contents=contents) # Restore only replaces the editable draft. Publishing remains an explicit # follow-up action so restoring history cannot unexpectedly activate it. @@ -2236,7 +2368,7 @@ class SkillManagementService: draft_files = [self._copy_draft_file(file, skill_id=skill.id) for file in files] filename = f"{skill.name}.zip" archive_bytes, _ = self._build_archive_from_draft(skill=draft_skill, files=draft_files) - return DraftSkillArchive(filename=filename, mime_type="application/zip", payload=archive_bytes) + return DraftSkillArchive(filename=filename, mime_type=_ZIP_MIME_TYPE, payload=archive_bytes) def pull_published_archive(self, *, tenant_id: str, skill_id: str) -> PublishedSkillArchive: with self._session_scope() as session: @@ -2252,10 +2384,22 @@ class SkillManagementService: ) tool_file_id = version.archive_tool_file_id filename = f"{skill.name}.zip" + return self._stored_archive(tenant_id=tenant_id, tool_file_id=tool_file_id, filename=filename) + + def export_version_archive(self, *, tenant_id: str, skill_id: str, version_id: str) -> PublishedSkillArchive: + with self._session_scope() as session: + skill = self._require_skill(session, tenant_id=tenant_id, skill_id=skill_id) + version = self._require_version(session, skill_id=skill.id, version_id=version_id) + tool_file_id = version.archive_tool_file_id + filename = f"{skill.name}-v{version.version_number}.zip" + return self._stored_archive(tenant_id=tenant_id, tool_file_id=tool_file_id, filename=filename) + + @classmethod + def _stored_archive(cls, *, tenant_id: str, tool_file_id: str, filename: str) -> PublishedSkillArchive: return PublishedSkillArchive( filename=filename, - mime_type="application/zip", - payload=self._load_tool_file_bytes(tenant_id=tenant_id, file_id=tool_file_id), + mime_type=_ZIP_MIME_TYPE, + payload=cls._load_tool_file_bytes(tenant_id=tenant_id, file_id=tool_file_id), ) def list_runtime_agent_skills( @@ -2275,7 +2419,7 @@ class SkillManagementService: "description": item.description, "size": item.size, "hash": item.hash, - "mime_type": "application/zip", + "mime_type": _ZIP_MIME_TYPE, } for item in self.list_runtime_agent_skill_archives( tenant_id=tenant_id, @@ -2394,7 +2538,7 @@ class SkillManagementService: filename = f"{published_name}.zip" return PublishedSkillArchive( filename=filename, - mime_type="application/zip", + mime_type=_ZIP_MIME_TYPE, payload=archive_bytes, ) @@ -3396,7 +3540,7 @@ class SkillManagementService: def _require_skill(session, *, tenant_id: str, skill_id: str) -> Skill: skill = session.scalar(select(Skill).where(Skill.tenant_id == tenant_id, Skill.id == skill_id)) if skill is None: - raise SkillManagementServiceError("skill_not_found", "skill not found", status_code=404) + raise SkillManagementServiceError(SkillServiceErrorCode.SKILL_NOT_FOUND, "skill not found", status_code=404) return skill @staticmethod @@ -3539,13 +3683,25 @@ class SkillManagementService: if skill_count is not None and skill_count >= _MAX_SKILLS_PER_WORKSPACE: raise SkillManagementServiceError("skill_limit_exceeded", "workspace skill limit exceeded") + @staticmethod + def _reject_taken_skill_name(session, *, tenant_id: str, name: str) -> None: + taken_id = session.scalar(select(Skill.id).where(Skill.tenant_id == tenant_id, Skill.name == name).limit(1)) + if taken_id is not None: + raise SkillManagementServiceError( + SkillServiceErrorCode.SKILL_NAME_CONFLICT, + "skill name already exists", + details={SkillErrorDetail.SKILL_ID: taken_id}, + ) + @staticmethod def _require_version(session, *, skill_id: str, version_id: str) -> SkillVersion: version = session.scalar( select(SkillVersion).where(SkillVersion.skill_id == skill_id, SkillVersion.id == version_id) ) if version is None: - raise SkillManagementServiceError("skill_version_not_found", "skill version not found", status_code=404) + raise SkillManagementServiceError( + SkillServiceErrorCode.SKILL_VERSION_NOT_FOUND, "skill version not found", status_code=404 + ) return version @staticmethod @@ -3809,21 +3965,35 @@ class SkillManagementService: # The explicit wrapper directory represents the new root, not a draft item. return {path: path.removeprefix(prefix) for path in paths if path.startswith(prefix)} - def _draft_payload_from_zip( - self, - *, - tenant_id: str, - user_id: str, - archive_bytes: bytes, - ) -> tuple[SkillDraftTreePayload, dict[str, Any], str]: + @classmethod + def _read_skill_package(cls, content: bytes) -> _SkillPackage: + """Read an uploaded skill zip and validate the name and description in its SKILL.md.""" + max_bytes = max_skill_archive_bytes() + if len(content) > max_bytes: + raise SkillManagementServiceError( + SkillServiceErrorCode.ARCHIVE_TOO_LARGE, f"skill archive is over {_size_text(max_bytes)}" + ) + contents = cls._read_skill_archive(archive_bytes=content) + return _SkillPackage( + contents=contents, + name=cls._require_frontmatter_name(contents.metadata, content=contents.skill_md_content), + description=cls._require_frontmatter_description(contents.metadata, content=contents.skill_md_content), + ) + + @classmethod + def _read_skill_archive(cls, *, archive_bytes: bytes) -> _SkillArchiveContents: try: with zipfile.ZipFile(io.BytesIO(archive_bytes)) as archive: - infos = self._validate_archive_limits(archive) + infos = [ + info + for info in cls._validate_archive_limits(archive) + if not is_os_junk(normalize_skill_file_path(info.filename.strip("/"))) + ] raw_paths = [normalize_skill_file_path(info.filename.strip("/")) for info in infos] - path_map = self._strip_single_root(raw_paths) + path_map = cls._strip_single_root(raw_paths) if _SKILL_MD not in set(path_map.values()): raise SkillManagementServiceError("missing_skill_md", "Skill package must contain SKILL.md") - items: list[SkillDraftTreeItemPayload] = [] + entries: list[SkillDraftTreeItemPayload | _BinaryArchiveEntry] = [] metadata: dict[str, Any] = {} skill_md_content = "" for info in infos: @@ -3832,47 +4002,168 @@ class SkillManagementService: continue path = normalize_skill_file_path(path_map[raw_path]) if info.is_dir(): - items.append(SkillDraftTreeItemPayload(path=path, kind=SkillFileKind.DIRECTORY)) + entries.append(SkillDraftTreeItemPayload(path=path, kind=SkillFileKind.DIRECTORY)) continue payload = archive.read(info) - text = self._decode_text_payload(path, payload) + text = cls._decode_text_payload(path, payload) if path == _SKILL_MD: if text is None: raise SkillManagementServiceError("invalid_skill_md", "SKILL.md must be UTF-8 text") - text = self._normalize_newlines(text) - metadata = self._parse_frontmatter(text) + text = cls._normalize_newlines(text) + metadata = cls._parse_frontmatter(text) skill_md_content = text - if text is not None: - items.append( - SkillDraftTreeItemPayload( - path=path, - storage=SkillFileStorage.TEXT, - mime_type=self._guess_mime_type(path), - content=text, - ) - ) - else: - tool_file = self._tool_files.create_file_by_raw( - user_id=user_id, - tenant_id=tenant_id, - conversation_id=None, - file_binary=payload, - mimetype=self._guess_mime_type(path), - filename=path.rsplit("/", 1)[-1], - ) - items.append( - SkillDraftTreeItemPayload( - path=path, - storage=SkillFileStorage.TOOL_FILE, - mime_type=self._guess_mime_type(path), - tool_file_id=tool_file.id, - size=len(payload), - hash=hashlib.sha256(payload).hexdigest(), - ) + if text is None: + entries.append(_BinaryArchiveEntry(path=path, payload=payload)) + continue + entries.append( + SkillDraftTreeItemPayload( + path=path, + storage=SkillFileStorage.TEXT, + mime_type=cls._guess_mime_type(path), + content=text, ) + ) except zipfile.BadZipFile as exc: raise SkillManagementServiceError("invalid_skill_package", "skill package must be a valid zip") from exc - return SkillDraftTreePayload(files=items), metadata, skill_md_content + return _SkillArchiveContents(entries=tuple(entries), metadata=metadata, skill_md_content=skill_md_content) + + def _store_archive_entries( + self, contents: _SkillArchiveContents, *, uploads: _ToolFileUploads + ) -> SkillDraftTreePayload: + items: list[SkillDraftTreeItemPayload] = [] + for entry in contents.entries: + if isinstance(entry, _BinaryArchiveEntry): + items.append(self._store_binary_entry(entry, uploads=uploads)) + continue + items.append(entry) + return SkillDraftTreePayload(files=items) + + def _store_binary_entry( + self, entry: _BinaryArchiveEntry, *, uploads: _ToolFileUploads + ) -> SkillDraftTreeItemPayload: + digest = hashlib.sha256(entry.payload).hexdigest() + size = len(entry.payload) + mime_type = self._guess_mime_type(entry.path) + tool_file_id = self._reusable_tool_file_id(uploads, digest=digest, size=size) + if tool_file_id is None: + tool_file = self._tool_files.create_file_by_raw( + user_id=uploads.user_id, + tenant_id=uploads.tenant_id, + conversation_id=None, + file_binary=entry.payload, + mimetype=mime_type, + filename=entry.path.rsplit("/", 1)[-1], + ) + uploads.created.append(tool_file) + tool_file_id = tool_file.id + return SkillDraftTreeItemPayload( + path=entry.path, + storage=SkillFileStorage.TOOL_FILE, + mime_type=mime_type, + tool_file_id=tool_file_id, + size=size, + hash=digest, + ) + + def _reusable_tool_file_id(self, uploads: _ToolFileUploads, *, digest: str, size: int) -> str | None: + for candidate_id in uploads.candidate_ids.get((digest, size), ()): + if candidate_id not in uploads.stored_digests: + uploads.stored_digests[candidate_id] = self._stored_tool_file_digest( + tenant_id=uploads.tenant_id, tool_file_id=candidate_id + ) + if uploads.stored_digests[candidate_id] == digest: + return candidate_id + return None + + @classmethod + def _stored_tool_file_digest(cls, *, tenant_id: str, tool_file_id: str) -> str | None: + try: + payload = cls._load_draft_tool_file_bytes(tenant_id=tenant_id, file_id=tool_file_id) + except Exception: + # Reuse is only an optimization; an unreadable candidate means storing a fresh copy. + logger.warning("Skill draft tool file %s could not be read for reuse", tool_file_id, exc_info=True) + return None + return hashlib.sha256(payload).hexdigest() + + def _replace_draft_from_archive( + self, + session, + *, + skill: Skill, + user_id: str, + contents: _SkillArchiveContents, + display_name: str | None = None, + ) -> list[SkillDraftFile]: + """``display_name`` replaces the skill's unless SKILL.md names one; None keeps the skill's. + + It is set only after the uploads, so no pending skill update holds a row lock while they run. + """ + uploads = _ToolFileUploads( + tenant_id=skill.tenant_id, + user_id=user_id, + candidate_ids=self._reuse_candidate_ids(session, skill=skill), + ) + with self._discard_uploads_on_failure(session, uploads=uploads): + draft_payload = self._store_archive_entries(contents, uploads=uploads) + if display_name is not None: + skill.display_name = display_name + files = self._build_draft_rows_from_tree( + skill=skill, + payload=draft_payload, + sync_frontmatter_name=True, + strict_frontmatter=True, + ) + self._replace_draft_rows(session, skill=skill, files=files, user_id=user_id) + return files + + @staticmethod + def _reuse_candidate_ids(session, *, skill: Skill) -> dict[tuple[str, int], tuple[str, ...]]: + """Draft rows whose tool file exists in the skill's tenant with the size the row claims.""" + rows = session.execute( + select(SkillDraftFile.hash, SkillDraftFile.size, SkillDraftFile.tool_file_id) + .join(ToolFile, ToolFile.id == SkillDraftFile.tool_file_id) + .where( + SkillDraftFile.skill_id == skill.id, + SkillDraftFile.storage == SkillFileStorage.TOOL_FILE, + SkillDraftFile.hash != "", + ToolFile.tenant_id == skill.tenant_id, + ToolFile.size == SkillDraftFile.size, + ) + ).all() + candidates: dict[tuple[str, int], tuple[str, ...]] = {} + for row in rows: + key = (row.hash, row.size) + candidates[key] = (*candidates.get(key, ()), row.tool_file_id) + return candidates + + @contextmanager + def _discard_uploads_on_failure(self, session, *, uploads: _ToolFileUploads) -> Generator[None]: + """Delete the tool files this request created when the block fails, then re-raise. + + Reused files are never in ``uploads.created``: other draft rows or features may reference them. + """ + try: + yield + except Exception: + if uploads.created: + self._delete_created_tool_files(session, uploads=uploads) + raise + + @staticmethod + def _delete_created_tool_files(session, *, uploads: _ToolFileUploads) -> None: + tool_file_ids = [tool_file.id for tool_file in uploads.created] + try: + # Release the failed transaction's row locks before the slower storage deletes. + session.rollback() + for tool_file in uploads.created: + with suppress(FileNotFoundError): + storage.delete(tool_file.file_key) + with session_factory.create_session() as cleanup_session, cleanup_session.begin(): + cleanup_session.execute( + delete(ToolFile).where(ToolFile.tenant_id == uploads.tenant_id, ToolFile.id.in_(tool_file_ids)) + ) + except Exception: + logger.exception("Failed to delete tool files stored by a failed skill archive request: %s", tool_file_ids) def _version_files_from_archive_bytes(self, archive_bytes: bytes) -> list[dict[str, Any]]: try: @@ -3935,7 +4226,7 @@ class SkillManagementService: """Validate ZIP metadata before any member is decompressed or persisted.""" infos = archive.infolist() if len(infos) > dify_config.SKILL_PACKAGE_MAX_ENTRIES: - raise SkillManagementServiceError("too_many_files", "skill file count limit exceeded") + raise _too_many_files_error() total_uncompressed = 0 for info in infos: @@ -3944,7 +4235,7 @@ class SkillManagementService: total_uncompressed += info.file_size if total_uncompressed > dify_config.SKILL_PACKAGE_MAX_UNCOMPRESSED_BYTES: - raise SkillManagementServiceError("skill_too_large", "skill exceeds uncompressed size limit") + raise _skill_too_large_error() if info.is_dir() or info.file_size == 0: continue @@ -3952,21 +4243,6 @@ class SkillManagementService: raise SkillManagementServiceError("invalid_skill_package", "skill package has invalid ZIP metadata") return infos - def _draft_rows_from_archive_bytes( - self, - *, - tenant_id: str, - user_id: str, - skill: Skill, - archive_bytes: bytes, - ) -> list[SkillDraftFile]: - payload, _metadata, _skill_md_content = self._draft_payload_from_zip( - tenant_id=tenant_id, - user_id=user_id, - archive_bytes=archive_bytes, - ) - return self._build_draft_rows_from_tree(skill=skill, payload=payload, sync_frontmatter_name=True) - @staticmethod def _draft_payload_items_from_rows(files: list[SkillDraftFile]) -> list[SkillDraftTreeItemPayload]: return [ @@ -4140,7 +4416,7 @@ class SkillManagementService: parent = posixpath.dirname(parent) if len(entries_by_path) > dify_config.SKILL_PACKAGE_MAX_ENTRIES: - raise SkillManagementServiceError("too_many_files", "skill file count limit exceeded") + raise _too_many_files_error() rows: list[SkillDraftFile] = [] total_size = 0 @@ -4173,7 +4449,7 @@ class SkillManagementService: ) if total_size > dify_config.SKILL_PACKAGE_MAX_UNCOMPRESSED_BYTES: - raise SkillManagementServiceError("skill_too_large", "skill exceeds uncompressed size limit") + raise _skill_too_large_error() return rows def _sync_skill_metadata_from_draft_skill_md(self, *, skill: Skill, content: str) -> None: @@ -4434,7 +4710,7 @@ class SkillManagementService: def _enforce_total_size(files: list[SkillDraftFile]) -> None: total = sum(file.size or 0 for file in {file.path: file for file in files}.values()) if total > dify_config.SKILL_PACKAGE_MAX_UNCOMPRESSED_BYTES: - raise SkillManagementServiceError("skill_too_large", "skill exceeds uncompressed size limit") + raise _skill_too_large_error() @staticmethod def _load_tool_file_bytes(*, tenant_id: str, file_id: str) -> bytes: @@ -4889,13 +5165,16 @@ __all__ = [ "SkillDraftFileOperationPayload", "SkillDraftTreeItemPayload", "SkillDraftTreePayload", + "SkillErrorDetail", "SkillImportPayload", "SkillManagementService", "SkillManagementServiceError", "SkillMetadataPayload", "SkillPublishPayload", "SkillRestorePayload", + "SkillServiceErrorCode", "SkillVersionUpdatePayload", + "max_skill_archive_bytes", "normalize_skill_file_path", "validate_skill_description", "validate_skill_name", diff --git a/api/tests/unit_tests/controllers/openapi/auth/test_requirements.py b/api/tests/unit_tests/controllers/openapi/auth/test_requirements.py index aab671f8276..b127edced82 100644 --- a/api/tests/unit_tests/controllers/openapi/auth/test_requirements.py +++ b/api/tests/unit_tests/controllers/openapi/auth/test_requirements.py @@ -27,11 +27,12 @@ from controllers.common.rbac import AgentBehindApp, PlainApp, RBACCheck, RBACPer from controllers.openapi import bp as openapi_bp from controllers.openapi._app_settings import REGULAR_MODES from controllers.openapi._catalog import CATALOG_HEADER, catalog_for -from controllers.openapi._contract import op_of +from controllers.openapi._contract import CapRequestBody, op_of from controllers.openapi._errors import ( OpenApiErrorCode, OpenApiErrorFormatter, ServiceApiDisabled, + SkillsDisabled, WebAppAccessRequiresEE, ) from controllers.openapi._models import ServiceApiPatch @@ -46,6 +47,7 @@ from controllers.openapi.auth.requirements import ( CheckPluginInstallSetting, CheckRBACPermission, CheckScope, + CheckSkillEnabled, CheckSubject, CheckWebAppAuthEnterprise, CheckWorkspaceInvitationQuota, @@ -180,6 +182,19 @@ class TestRequirementsCases: CheckAppApiEnabled().run(subject, make_ctx(sqlite_session, subject=subject, app_id=APP_ID), sqlite_session) + @staticmethod + def test_check_skill_enabled_follows_the_server_flag( + sqlite_session: Session, config_overrides: Callable[..., None] + ) -> None: + subject = account_subject() + ctx = make_ctx(sqlite_session, subject=subject) + + config_overrides(ENABLE_SKILL=True) + CheckSkillEnabled().run(subject, ctx, sqlite_session) + config_overrides(ENABLE_SKILL=False) + with pytest.raises(SkillsDisabled): + CheckSkillEnabled().run(subject, ctx, sqlite_session) + class TestCheckRBACPermission: @staticmethod def _requirement() -> CheckRBACPermission: @@ -642,6 +657,26 @@ ROUTES: tuple[Route, ...] = ( "/workspaces/{workspace_id}/tool-providers/{provider}/credentials/{credential_id}", frozenset({Trait.ACCOUNT_PRIMARY}), ), + Route("skills.list", "GET", "/workspaces/{workspace_id}/skills", frozenset({Trait.ACCOUNT_PRIMARY})), + Route("skills.describe", "GET", "/workspaces/{workspace_id}/skills/{skill_id}", frozenset({Trait.ACCOUNT_PRIMARY})), + Route( + "skills.versions", + "GET", + "/workspaces/{workspace_id}/skills/{skill_id}/versions", + frozenset({Trait.ACCOUNT_PRIMARY}), + ), + Route( + "skills.export", + "GET", + "/workspaces/{workspace_id}/skills/{skill_id}/archive", + frozenset({Trait.ACCOUNT_PRIMARY}), + ), + Route( + "skills.publish", + "POST", + "/workspaces/{workspace_id}/skills/{skill_id}:publish", + frozenset({Trait.ACCOUNT_PRIMARY}), + ), Route( "knowledge_bases.list", "GET", @@ -666,6 +701,7 @@ ROUTES: tuple[Route, ...] = ( "/workspaces/{workspace_id}/model-providers/{provider}/models/credentials/{credential_id}", frozenset({Trait.ACCOUNT_PRIMARY}), ), + Route("skills.import", "POST", "/workspaces/{workspace_id}/skills/imports", frozenset({Trait.ACCOUNT_PRIMARY})), Route( "app_dsl.import", "POST", @@ -1310,6 +1346,9 @@ MATRIX: dict[str, dict[Case, Expect]] = { "workspaces.members.invite": dict(_ACCOUNT_MEMBER_WITH_ROLE), "workspaces.members.update_role": dict(_ACCOUNT_MEMBER_WITH_ROLE), "plugins.install": {**_ACCOUNT_MEMBER_NO_ROLE, Case.RBAC_ON_DENIED: DENY_RBAC}, + "skills.list": {**_ACCOUNT_MEMBER_NO_ROLE, Case.RBAC_ON_DENIED: DENY_RBAC}, + "skills.publish": dict(_ACCOUNT_MEMBER_WITH_ROLE), + "skills.import": dict(_ACCOUNT_MEMBER_WITH_ROLE), "model_providers.credential.create": dict(_ACCOUNT_MEMBER_WITH_ROLE), "app_dsl.import": dict(_ACCOUNT_MEMBER_WITH_ROLE), "app_dsl.check": dict(_ACCOUNT_MEMBER_WITH_ROLE), @@ -1395,6 +1434,11 @@ _REQ_ACCOUNT_WORKSPACE_READ_MEMBER = ( CheckScope(Scope.WORKSPACE_READ), CheckWorkspaceMember(), ) +_REQ_SKILL_READ = ( + CheckSkillEnabled(), + *_REQ_ACCOUNT_WORKSPACE_READ_MEMBER, + CheckRBACPermission(RBACCheck(RBACPermission.SKILL_VIEW, Workspace())), +) _REQ_MEMBER_MANAGE = ( CheckSubject(allowed=_ACCOUNT), CheckScope(Scope.WORKSPACE_WRITE), @@ -1537,6 +1581,17 @@ def _req_admin_write(permission: RBACPermission) -> tuple[Requirement, ...]: ) +def _req_skill_write(permission: RBACPermission) -> tuple[Requirement, ...]: + return ( + CheckSkillEnabled(), + CheckSubject(allowed=_ACCOUNT), + CheckScope(Scope.WORKSPACE_WRITE), + CheckWorkspaceMember(), + CheckRBACPermission(RBACCheck(permission, Workspace())), + CheckWorkspaceRole(_EDITOR_UP), + ) + + _REQ_PLUGIN_TASK = ( CheckSubject(allowed=_ACCOUNT), CheckScope(Scope.WORKSPACE_READ), @@ -1600,6 +1655,15 @@ DECLARED: dict[str, tuple[Requirement, ...]] = { "tool_providers.describe": _REQ_ACCOUNT_WORKSPACE_READ_MEMBER, "tool_providers.credential.create": _req_admin_write(RBACPermission.CREDENTIAL_CREATE), "tool_providers.credential.set": _req_admin_write(RBACPermission.CREDENTIAL_MANAGE), + "skills.list": _REQ_SKILL_READ, + "skills.describe": _REQ_SKILL_READ, + "skills.versions": _REQ_SKILL_READ, + "skills.export": _REQ_SKILL_READ, + "skills.publish": _req_skill_write(RBACPermission.SKILL_PUBLISH), + "skills.import": ( + *_req_skill_write(RBACPermission.SKILL_EDIT), + CapRequestBody(max_bytes=lambda: 0, max_parts=lambda: 0), + ), "knowledge_bases.list": _REQ_ACCOUNT_WORKSPACE_READ_MEMBER, "models.list": _REQ_ACCOUNT_WORKSPACE_READ_MEMBER, "models.credential.create": _req_admin_write(RBACPermission.CREDENTIAL_CREATE), @@ -1930,6 +1994,7 @@ def _url(route: Route, world: World, scenario: Scenario, bearer: Bearer | None) "form_token": uuid.uuid4().hex, "run_id": str(uuid.uuid4()), "version_id": str(uuid.uuid4()), + "skill_id": str(uuid.uuid4()), "env_id": str(uuid.uuid4()), "node_type": "llm", "node_id": "node-1", @@ -2137,6 +2202,12 @@ EXPECTED_RESPONSE_CODES: dict[tuple[str, str], frozenset[str]] = { {"200", "422", "default"} ), ("get", "/workspaces/{workspace_id}/knowledge-bases"): frozenset({"200", "422", "default"}), + ("get", "/workspaces/{workspace_id}/skills"): frozenset({"200", "422", "default"}), + ("get", "/workspaces/{workspace_id}/skills/{skill_id}"): frozenset({"200", "default"}), + ("get", "/workspaces/{workspace_id}/skills/{skill_id}/versions"): frozenset({"200", "422", "default"}), + ("get", "/workspaces/{workspace_id}/skills/{skill_id}/archive"): frozenset({"200", "422", "default"}), + ("post", "/workspaces/{workspace_id}/skills/{skill_id}:publish"): frozenset({"200", "422", "default"}), + ("post", "/workspaces/{workspace_id}/skills/imports"): frozenset({"200", "201", "422", "default"}), ("get", "/workspaces/{workspace_id}/models"): frozenset({"200", "422", "default"}), ("post", "/workspaces/{workspace_id}/model-providers/{provider}/models/credentials"): frozenset( {"201", "422", "default"} diff --git a/api/tests/unit_tests/controllers/openapi/test__contract.py b/api/tests/unit_tests/controllers/openapi/test__contract.py index 9443631e7b4..a34d2846541 100644 --- a/api/tests/unit_tests/controllers/openapi/test__contract.py +++ b/api/tests/unit_tests/controllers/openapi/test__contract.py @@ -18,7 +18,14 @@ from werkzeug.exceptions import RequestEntityTooLarge, UnprocessableEntity from controllers.common.schema import register_response_schema_model, register_schema_model from controllers.openapi import openapi_ns -from controllers.openapi._contract import CapRequestBody, accepts, paginated, returns +from controllers.openapi._contract import ( + MULTIPART_HEADER_BYTES_PER_PART, + CapRequestBody, + accepts, + multipart_header_allowance, + paginated, + returns, +) from controllers.openapi._errors import RequestOverRouteLimits from controllers.openapi._models import Hint, PaginationEnvelope from controllers.openapi._upload import UploadPart @@ -129,6 +136,23 @@ def test_accepts_injects_validated_query_with_defaults_for_absent_fields(app): assert result.limit == 20 +class ContractListQuery(BaseModel): + tag: list[str] | None = None + word: str | None = None + + +def test_accepts_fills_list_typed_query_fields_from_repeated_parameters(app: Flask) -> None: + @accepts(query=ContractListQuery) + def view(*, query: ContractListQuery) -> ContractListQuery: + return query + + with app.test_request_context("/?tag=a&tag=b&word=x&word=y"): + result = view() + + assert result.tag == ["a", "b"] + assert result.word == "x" + + @pytest.mark.parametrize("query_string", ["page=abc", "unknown=1"]) def test_accepts_rejects_invalid_query_with_422(app, query_string): @accepts(query=ContractQuery) @@ -278,3 +302,8 @@ def test_an_uncapped_route_keeps_the_generic_413(app: Flask, view: Callable[[], with _post(app, 1, extra="y"), pytest.raises(RequestEntityTooLarge) as exc: view() assert not isinstance(exc.value, RequestOverRouteLimits) + + +def test_multipart_header_allowance_scales_with_the_part_count() -> None: + assert multipart_header_allowance(0) == 0 + assert multipart_header_allowance(5002) == 5002 * MULTIPART_HEADER_BYTES_PER_PART diff --git a/api/tests/unit_tests/controllers/openapi/test__multipart.py b/api/tests/unit_tests/controllers/openapi/test__multipart.py index 59229e466d5..3fc708e1fa9 100644 --- a/api/tests/unit_tests/controllers/openapi/test__multipart.py +++ b/api/tests/unit_tests/controllers/openapi/test__multipart.py @@ -9,7 +9,8 @@ from flask.ctx import RequestContext from werkzeug.datastructures import FileStorage from controllers.openapi._errors import InvalidFilePart -from controllers.openapi._multipart import body_from_request +from controllers.openapi._multipart import body_from_request, folder_streams +from controllers.openapi._upload import UploadParts _FILE_FIELDS = frozenset({"file", "files", "attachments"}) @@ -68,6 +69,18 @@ def test_multipart_rejects_malformed_bodies(app: Flask, data: Mapping[str, objec body_from_request(file_fields=_FILE_FIELDS) +def test_folder_streams_maps_each_path_to_its_stream() -> None: + first, second = FileStorage(stream=BytesIO(b"a")), FileStorage(stream=BytesIO(b"b")) + streams = folder_streams({"SKILL.md": first, "scripts/run.py": second}) + assert streams == {"SKILL.md": first.stream, "scripts/run.py": second.stream} + + +def test_folder_streams_rejects_a_path_given_more_than_once() -> None: + parts: UploadParts = {"SKILL.md": [FileStorage(stream=BytesIO(b"a")), FileStorage(stream=BytesIO(b"b"))]} + with pytest.raises(InvalidFilePart, match="SKILL.md"): + folder_streams(parts) + + @pytest.mark.parametrize("direct", ["files", "files[]"]) @pytest.mark.parametrize("mapped", ["files[doc]", "files[doc][]"]) @pytest.mark.parametrize("mapped_first", [False, True], ids=["direct_first", "mapped_first"]) diff --git a/api/tests/unit_tests/controllers/openapi/test_skills.py b/api/tests/unit_tests/controllers/openapi/test_skills.py new file mode 100644 index 00000000000..ad99b17caf3 --- /dev/null +++ b/api/tests/unit_tests/controllers/openapi/test_skills.py @@ -0,0 +1,501 @@ +from __future__ import annotations + +import io +import zipfile +from collections.abc import Callable, Mapping +from typing import Protocol, cast + +import pytest +from flask import Flask, Response +from flask.ctx import RequestContext as FlaskRequestContext +from flask_restx import Resource +from pydantic import ValidationError +from sqlalchemy.orm import Session +from werkzeug.datastructures import FileStorage +from werkzeug.exceptions import UnprocessableEntity + +from configs import dify_config +from constants import BYTES_PER_MIB +from controllers.openapi._contract import accepts +from controllers.openapi._errors import ( + DraftChanged, + InvalidFilePart, + OpenApiError, + OpenApiErrorCode, + RequestOverRouteLimits, + SkillNameConflict, + SkillNameMismatch, + SkillNotFound, + SkillPackageInvalid, + SkillPackageTooLarge, + SkillRequestRejected, + SkillsDisabled, + SkillVersionNotFound, +) +from controllers.openapi._models import ( + SkillDescribeResponse, + SkillExportQuery, + SkillImportPayload, + SkillImportResponse, + SkillListQuery, + SkillListResponse, + SkillPublishPayload, + SkillPublishResponse, + SkillVersionListQuery, + SkillVersionListResponse, +) +from controllers.openapi._upload import UploadParts +from controllers.openapi.auth.pipelines import Pipeline +from controllers.openapi.auth.spec import EndpointSpec +from controllers.openapi.skills import ( + _UPLOAD_CAP, + ServiceRejection, + SkillDescribeApi, + SkillExportApi, + SkillImportApi, + SkillListApi, + SkillPublishApi, + SkillVersionListApi, + _draft_hash, + _expected_updated_at, + _rejection, + _skill_upload_parts, +) +from machinery.context import RequestContext +from services.skill_management_service import ( + PublishedSkillArchive, + SkillErrorDetail, + SkillManagementService, + SkillManagementServiceError, + SkillOverwritePayload, + SkillServiceErrorCode, +) +from services.skill_management_service import SkillImportPayload as ServiceSkillImportPayload +from services.skill_management_service import SkillPublishPayload as ServiceSkillPublishPayload +from tests.helpers.unit.controllers.openapi.auth._world import TENANT_ID, account_subject, make_ctx, never_reached + +WORKSPACE_ID = "ws-id" +SKILL_ID = "11111111-1111-4111-8111-111111111111" +EXISTING_ID = "22222222-2222-4222-8222-222222222222" +UPDATED_AT = 1761200000 +SKILL_MD = {"SKILL.md": b"x"} + + +type Record = dict[str, object] + + +class Endpoint[T](Protocol): + __handler__: Callable[..., T] + __spec__: EndpointSpec + + +def _context() -> RequestContext: + return RequestContext(request_id="r", trace_id=None, account_id="acct-id", active_workspace_id=WORKSPACE_ID) + + +def _skill(**extra: object) -> Record: + return { + "id": SKILL_ID, + "name": "expense-sop", + "display_name": "Expense SOP", + "description": "How to file expenses", + "icon": None, + "tags": ["ops"], + "latest_published_version_id": "ver-2", + "updated_at": UPDATED_AT, + "files": [{"path": "SKILL.md"}], + **extra, + } + + +def _version(number: int, *, latest: bool = False) -> Record: + return { + "id": f"ver-{number}", + "version_number": number, + "version_name": None, + "publish_note": "", + "published_by": "acct-id", + "is_latest": latest, + "created_at": UPDATED_AT + number, + } + + +def _stub(monkeypatch: pytest.MonkeyPatch, name: str, replacement: Callable[..., object]) -> None: + def stand_in(_service: SkillManagementService, **kwargs: object) -> object: + return replacement(**kwargs) + + monkeypatch.setattr(SkillManagementService, name, stand_in) + + +def _record(monkeypatch: pytest.MonkeyPatch, name: str, result: object) -> list[Record]: + """Stub a service method that returns ``result`` and keeps the keyword arguments of each call.""" + seen: list[Record] = [] + + def record(**kwargs: object) -> object: + seen.append(kwargs) + return result + + _stub(monkeypatch, name, record) + return seen + + +def _reject(code: str, **details: object) -> Callable[..., object]: + def reject(**_kwargs: object) -> object: + raise SkillManagementServiceError(code, "rejected", details=details) + + return reject + + +def _call(api: Resource, view: object, *args: object, **kwargs: object) -> object: + return cast(Endpoint[object], view).__handler__(api, _context(), WORKSPACE_ID, *args, **kwargs) + + +def _folder(files: Mapping[str, bytes]) -> UploadParts: + return {path: FileStorage(stream=io.BytesIO(data)) for path, data in files.items()} + + +def _import(files: Mapping[str, bytes], **fields: object) -> tuple[SkillImportResponse, int]: + body = SkillImportPayload.model_validate({"files": _folder(files), **fields}) + return cast(tuple[SkillImportResponse, int], _call(SkillImportApi(), SkillImportApi.post, body=body)) + + +def _archive(filename: str) -> PublishedSkillArchive: + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + archive.writestr("expense-sop/SKILL.md", "---\nname: expense-sop\n---\n") + return PublishedSkillArchive(filename=filename, mime_type="application/zip", payload=buffer.getvalue()) + + +_HANDLER_CALLS: Mapping[str, Callable[[], object]] = { + "get_skill": lambda: _call(SkillDescribeApi(), SkillDescribeApi.get, SKILL_ID), + "export_version_archive": lambda: _call( + SkillExportApi(), SkillExportApi.get, SKILL_ID, query=SkillExportQuery(version_id=SKILL_ID) + ), + "publish_skill": lambda: _call(SkillPublishApi(), SkillPublishApi.post, SKILL_ID, body=SkillPublishPayload()), + "import_skill": lambda: _import(SKILL_MD), + "overwrite_skill": lambda: _import(SKILL_MD, skill_id=SKILL_ID), +} + + +def test_every_mapped_code_is_a_service_code() -> None: + assert {rejection.value for rejection in ServiceRejection} <= {code.value for code in SkillServiceErrorCode} + + +def test_rejection_relays_unknown_codes_with_their_status() -> None: + error = _rejection(SkillManagementServiceError("brand_new_code", "msg", status_code=422, details={"a": 1})) + assert isinstance(error, SkillRequestRejected) + assert (error.code, error.description) == (422, "msg") + assert error.details is not None + assert [(d.type, d.loc, d.msg) for d in error.details] == [ + ("brand_new_code", [], "msg"), + ("brand_new_code", ["a"], "1"), + ] + + +@pytest.mark.parametrize( + ("method", "code", "error_type", "status"), + [ + ("get_skill", SkillServiceErrorCode.SKILL_NOT_FOUND, SkillNotFound, 404), + ("export_version_archive", SkillServiceErrorCode.SKILL_VERSION_NOT_FOUND, SkillVersionNotFound, 404), + ("overwrite_skill", SkillServiceErrorCode.SKILL_CONFLICT, DraftChanged, 409), + ("import_skill", SkillServiceErrorCode.SKILL_NAME_CONFLICT, SkillNameConflict, 409), + ("overwrite_skill", SkillServiceErrorCode.SKILL_NAME_MISMATCH, SkillNameMismatch, 400), + ("publish_skill", SkillServiceErrorCode.INVALID_SKILL_MD, SkillPackageInvalid, 400), + ("overwrite_skill", SkillServiceErrorCode.MISSING_SKILL_NAME, SkillPackageInvalid, 400), + ("overwrite_skill", SkillServiceErrorCode.INVALID_SKILL_NAME, SkillPackageInvalid, 400), + *(("import_skill", code, code.error, 400) for code in ServiceRejection if code.error is SkillPackageInvalid), + *(("import_skill", code, code.error, 413) for code in ServiceRejection if code.error is SkillPackageTooLarge), + ], + ids=lambda value: value.__name__ if isinstance(value, type) else None, +) +def test_handlers_answer_a_service_rejection_with_its_error_and_message( + app: Flask, monkeypatch: pytest.MonkeyPatch, method: str, code: str, error_type: type[OpenApiError], status: int +) -> None: + _stub(monkeypatch, method, _reject(code)) + with app.test_request_context(), pytest.raises(error_type) as exc: + _HANDLER_CALLS[method]() + assert (exc.value.code, exc.value.description) == (status, "rejected") + + +def test_draft_hash_is_epoch_seconds() -> None: + assert _draft_hash(1761200000) == "1761200000" + assert _expected_updated_at("1761200000") == 1761200000 + assert _expected_updated_at(None) is None + + +@pytest.mark.parametrize( + ("api", "view", "kwargs"), + [ + (SkillDescribeApi, SkillDescribeApi.get, {}), + (SkillVersionListApi, SkillVersionListApi.get, {"query": SkillVersionListQuery(page=1, limit=2)}), + (SkillExportApi, SkillExportApi.get, {"query": SkillExportQuery()}), + (SkillPublishApi, SkillPublishApi.post, {"body": SkillPublishPayload()}), + ], +) +def test_non_uuid_skill_id_is_404(api: type[Resource], view: object, kwargs: dict[str, object]) -> None: + with pytest.raises(SkillNotFound): + _call(api(), view, "not-a-uuid", **kwargs) + + +def test_list_skills_passes_filters_and_pages(monkeypatch: pytest.MonkeyPatch) -> None: + page = {"data": [_skill()], "total": 11, "page": 2, "limit": 5, "has_more": True} + seen = _record(monkeypatch, "list_skills", page) + query = SkillListQuery.model_validate({"keyword": "sop", "tag": ["ops"], "page": 2, "limit": 5}) + result = cast(SkillListResponse, _call(SkillListApi(), SkillListApi.get, query=query)) + assert seen == [{"tenant_id": WORKSPACE_ID, "keyword": "sop", "tags": ["ops"], "page": 2, "limit": 5}] + assert (result.page, result.limit, result.total, result.has_more) == (2, 5, 11, True) + assert result.data[0].name == "expense-sop" + + +@pytest.mark.parametrize( + ("versions", "latest"), + [([_version(2, latest=True), _version(1)], {"id": "ver-2", "created_by": "acct-id"}), ([], None)], + ids=["published", "never-published"], +) +def test_describe_skill_returns_draft_hash_and_latest_version( + monkeypatch: pytest.MonkeyPatch, versions: list[Record], latest: Record | None +) -> None: + _stub(monkeypatch, "get_skill", lambda **_: _skill()) + _stub(monkeypatch, "list_versions", lambda **_: {"data": versions}) + result = cast(SkillDescribeResponse, _call(SkillDescribeApi(), SkillDescribeApi.get, SKILL_ID)) + assert result.draft_hash == str(UPDATED_AT) + assert result.model_dump(include={"latest_version": {"id", "created_by"}}) == {"latest_version": latest} + assert "files" not in result.model_dump() + + +def test_list_versions_newest_first_and_paged(monkeypatch: pytest.MonkeyPatch) -> None: + _stub(monkeypatch, "list_versions", lambda **_: {"data": [_version(3, latest=True), _version(2), _version(1)]}) + query = SkillVersionListQuery(page=1, limit=2) + result = cast( + SkillVersionListResponse, _call(SkillVersionListApi(), SkillVersionListApi.get, SKILL_ID, query=query) + ) + assert [row.version_number for row in result.data] == [3, 2] + assert (result.has_more, result.total) == (True, 3) + + +@pytest.mark.parametrize( + ("method", "version", "filename"), + [("export_draft_archive", {}, "expense-sop.zip"), ("export_version_archive", {"version_id": SKILL_ID}, "v2.zip")], + ids=["draft", "version"], +) +def test_export_sends_the_zip_as_an_attachment( + app: Flask, monkeypatch: pytest.MonkeyPatch, method: str, version: dict[str, str], filename: str +) -> None: + seen = _record(monkeypatch, method, _archive(filename)) + query = SkillExportQuery.model_validate(version) + with app.test_request_context(): + response = cast(Response, _call(SkillExportApi(), SkillExportApi.get, SKILL_ID, query=query)) + assert seen == [{"tenant_id": WORKSPACE_ID, "skill_id": SKILL_ID, **version}] + assert response.mimetype == "application/zip" + assert response.headers["Content-Disposition"] == f"attachment; filename={filename}" + response.direct_passthrough = False + assert zipfile.ZipFile(io.BytesIO(response.get_data())).namelist() == ["expense-sop/SKILL.md"] + + +def test_publish_passes_name_and_note(monkeypatch: pytest.MonkeyPatch) -> None: + seen = _record(monkeypatch, "publish_skill", _version(2, latest=True)) + body = SkillPublishPayload(version_name="v2", publish_note="retry") + result = cast(SkillPublishResponse, _call(SkillPublishApi(), SkillPublishApi.post, SKILL_ID, body=body)) + assert seen == [ + { + "tenant_id": WORKSPACE_ID, + "user_id": "acct-id", + "skill_id": SKILL_ID, + "payload": ServiceSkillPublishPayload(version_name="v2", publish_note="retry"), + } + ] + assert (result.version_number, result.created_by) == (2, "acct-id") + + +def test_import_folder_creates_skill(monkeypatch: pytest.MonkeyPatch) -> None: + seen = _record(monkeypatch, "import_skill", _skill(files=[{"path": "SKILL.md"}, {"path": "scripts/run.py"}])) + response, status = _import({"SKILL.md": b"---\nname: expense-sop\n---\n", "scripts/run.py": b"x"}) + assert status == 201 + [sent] = seen + assert (sent["tenant_id"], sent["user_id"]) == (WORKSPACE_ID, "acct-id") + payload = cast(ServiceSkillImportPayload, sent["payload"]) + assert zipfile.ZipFile(io.BytesIO(payload.content)).namelist() == ["SKILL.md", "scripts/run.py"] + expected = (SKILL_ID, "expense-sop", str(UPDATED_AT), 2) + assert (response.id, response.name, response.draft_hash, response.file_count) == expected + [hint] = response.hints + assert (hint.op, hint.input) == ("publish.skill", {"workspace_id": WORKSPACE_ID, "skill_id": SKILL_ID}) + + +def test_import_with_skill_id_overwrites(monkeypatch: pytest.MonkeyPatch) -> None: + seen = _record(monkeypatch, "overwrite_skill", _skill()) + _stub(monkeypatch, "import_skill", lambda **_: pytest.fail("must not create")) + response, status = _import(SKILL_MD, skill_id=SKILL_ID, draft_hash="1761200000") + assert status == 200 + [sent] = seen + assert sent["skill_id"] == SKILL_ID + assert cast(SkillOverwritePayload, sent["payload"]).expected_updated_at == 1761200000 + assert response.hints[0].op == "publish.skill" + + +@pytest.mark.parametrize( + ("fields", "loc"), + [ + ({"skill_id": SKILL_ID, "draft_hash": "abc"}, "draft_hash"), + ({"skill_id": SKILL_ID, "draft_hash": "١٢٣"}, "draft_hash"), + ({"skill_id": "not-a-uuid"}, "skill_id"), + ({"draft_hash": "1761200000"}, "draft_hash"), + ], + ids=["letters-draft-hash", "non-ascii-digits-draft-hash", "non-uuid-skill-id", "draft-hash-without-skill-id"], +) +def test_import_payload_rejects_a_bad_field(fields: Record, loc: str) -> None: + with pytest.raises(ValidationError) as exc: + SkillImportPayload.model_validate({"files": _folder(SKILL_MD), **fields}) + assert [error["loc"] for error in exc.value.errors()] == [(loc,)] + + +@pytest.mark.parametrize( + ("method", "details", "hints"), + [ + ( + "import_skill", + {SkillErrorDetail.SKILL_ID: EXISTING_ID}, + [("import.skill", {"workspace_id": WORKSPACE_ID, "skill_id": EXISTING_ID, "files": None})], + ), + ( + "import_skill", + {SkillErrorDetail.NAME: "expense-sop"}, + [("get.skill", {"workspace_id": WORKSPACE_ID, "keyword": "expense-sop"})], + ), + ("overwrite_skill", {}, []), + ], + ids=["pre-check-overwrites-the-existing-skill", "commit-race-searches-by-name", "nothing-to-point-at"], +) +def test_import_name_conflict_hints_at_the_clashing_skill( + monkeypatch: pytest.MonkeyPatch, method: str, details: Record, hints: list[tuple[str, Record]] +) -> None: + _stub(monkeypatch, method, _reject(SkillServiceErrorCode.SKILL_NAME_CONFLICT, **details)) + with pytest.raises(SkillNameConflict) as exc: + _HANDLER_CALLS[method]() + assert exc.value.error_code == OpenApiErrorCode.SKILL_NAME_CONFLICT + assert exc.value.hints is not None + assert [(hint.op, hint.input) for hint in exc.value.hints] == hints + + +@pytest.mark.parametrize( + ("limit", "files", "message"), + [ + ( + {"UPLOAD_SKILL_FILE_SIZE_LIMIT": 1}, + {"SKILL.md": b"x" * (2 * BYTES_PER_MIB)}, + "The skill folder is over 1 MiB", + ), + ( + {"SKILL_PACKAGE_MAX_ENTRIES": 2}, + {"SKILL.md": b"x", "a.txt": b"x", "b.txt": b"x"}, + "The skill folder has more than 2 files", + ), + ], + ids=["bytes", "files"], +) +def test_import_folder_over_a_limit_is_413_naming_the_limit( + monkeypatch: pytest.MonkeyPatch, + config_overrides: Callable[..., None], + limit: Record, + files: Mapping[str, bytes], + message: str, +) -> None: + config_overrides(**limit) + _stub(monkeypatch, "import_skill", lambda **_: pytest.fail("must not reach the service")) + with pytest.raises(SkillPackageTooLarge) as exc: + _import(files) + assert (exc.value.code, exc.value.description) == (413, message) + + +def test_import_part_cap_counts_files_and_fields( + app: Flask, sqlite_session: Session, monkeypatch: pytest.MonkeyPatch +) -> None: + assert _skill_upload_parts() == dify_config.SKILL_PACKAGE_MAX_ENTRIES + 2 + monkeypatch.setattr(dify_config, "SKILL_PACKAGE_MAX_ENTRIES", 2) + assert _skill_upload_parts() == 4 + + @accepts(body=SkillImportPayload) + def handler(*, body: SkillImportPayload) -> int: + return len(body.files) + + def post(*paths: str) -> FlaskRequestContext: + data: dict[str, object] = { + "skill_id": f'"{SKILL_ID}"', + "draft_hash": '"1761200000"', + **{f"files[{path}]": (io.BytesIO(b"x"), path) for path in paths}, + } + return app.test_request_context("/x", method="POST", data=data, content_type="multipart/form-data") + + with post("SKILL.md", "a.txt"): + _UPLOAD_CAP.run(account_subject(), make_ctx(sqlite_session), sqlite_session) + assert handler() == 2 + with post("SKILL.md", "a.txt", "b.txt"): + _UPLOAD_CAP.run(account_subject(), make_ctx(sqlite_session), sqlite_session) + with pytest.raises(RequestOverRouteLimits, match="at most 4 parts"): + handler() + + +def test_import_bad_path_names_the_path() -> None: + with pytest.raises(SkillPackageInvalid) as exc: + _import({"SKILL.md": b"x", "a/../b": b"x"}) + assert exc.value.details is not None + assert any(detail.loc == ["a/../b"] for detail in exc.value.details) + assert "a/../b" in str(exc.value.description) + + +def test_import_only_junk_is_rejected() -> None: + with pytest.raises(SkillPackageInvalid): + _import({".DS_Store": b"x"}) + + +@pytest.mark.parametrize( + ("data", "error_type", "match"), + [ + ( + {"files[SKILL.md]": [(io.BytesIO(b"a"), "SKILL.md"), (io.BytesIO(b"b"), "SKILL.md")]}, + InvalidFilePart, + "SKILL.md", + ), + ({"files[SKILL.md]": (io.BytesIO(b"x"), "SKILL.md"), "force": "true"}, UnprocessableEntity, None), + ], + ids=["same-path-twice", "unknown-field"], +) +def test_import_body_is_rejected_before_parsing( + app: Flask, data: Record, error_type: type[Exception], match: str | None +) -> None: + @accepts(body=SkillImportPayload) + def handler(**_kwargs: object) -> None: + pytest.fail("must not parse") + + with ( + app.test_request_context("/x", method="POST", data=data, content_type="multipart/form-data"), + pytest.raises(error_type, match=match), + ): + handler() + + +@pytest.mark.parametrize( + "view", + [ + SkillListApi.get, + SkillDescribeApi.get, + SkillVersionListApi.get, + SkillExportApi.get, + SkillPublishApi.post, + SkillImportApi.post, + ], +) +def test_skills_off_is_503_before_membership_and_permission_checks( + sqlite_session: Session, config_overrides: Callable[..., None], view: object +) -> None: + """The caller is not a member of the workspace, so any earlier check would refuse it first.""" + config_overrides(ENABLE_SKILL=False, RBAC_ENABLED=True) + subject = account_subject() + with pytest.raises(SkillsDisabled): + Pipeline().run( + subject=subject, + auth=subject.auth, + spec=cast(Endpoint[object], view).__spec__, + ctx=make_ctx(sqlite_session, subject=subject, workspace_id=TENANT_ID), + session=sqlite_session, + call=never_reached, + ) diff --git a/api/tests/unit_tests/services/test_skill_management_service.py b/api/tests/unit_tests/services/test_skill_management_service.py index 0cfece6349b..49dc21e6e67 100644 --- a/api/tests/unit_tests/services/test_skill_management_service.py +++ b/api/tests/unit_tests/services/test_skill_management_service.py @@ -5,15 +5,16 @@ from __future__ import annotations import io import json import zipfile -from collections.abc import Generator +from collections.abc import Callable, Generator, Mapping from types import SimpleNamespace from typing import cast, override from unittest.mock import patch from uuid import uuid4 import pytest -from sqlalchemy import Table, delete, func, select +from sqlalchemy import Table, delete, func, select, update from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import Session from core.credit_usage import CreditUsageCreatedBy from core.db.session_factory import session_factory @@ -61,12 +62,15 @@ from services.skill_management_service import ( SkillDraftFileOperationPayload, SkillDraftTreeItemPayload, SkillDraftTreePayload, + SkillErrorDetail, SkillImportPayload, SkillManagementService, SkillManagementServiceError, SkillMetadataPayload, + SkillOverwritePayload, SkillPublishPayload, SkillRestorePayload, + SkillServiceErrorCode, SkillVersionUpdatePayload, normalize_skill_file_path, validate_skill_description, @@ -80,6 +84,10 @@ USER = "33333333-3333-3333-3333-333333333333" class _FakeToolFileManager(ToolFileManager): + def __init__(self) -> None: + super().__init__() + self.stored: dict[str, bytes] = {} + @override def create_file_by_raw( self, @@ -105,6 +113,7 @@ class _FakeToolFileManager(ToolFileManager): with session_factory.create_session() as session: session.add(tool_file) session.commit() + self.stored[tool_file.file_key] = file_binary return tool_file @@ -2941,7 +2950,7 @@ def test_import_skill_package_rejects_archive_larger_than_upload_skill_limit(mon payload=SkillImportPayload(content=b"not-empty", filename="expense-sop.zip"), ) - assert exc_info.value.code == "archive_too_large" + assert (exc_info.value.code, exc_info.value.message) == ("archive_too_large", "skill archive is over 0 MiB") def test_import_skill_package_rejects_excessive_expansion_before_reading_members( @@ -2958,13 +2967,9 @@ def test_import_skill_package_rejects_excessive_expansion_before_reading_members patch.object(zipfile.ZipFile, "read", side_effect=AssertionError("member read before size validation")), pytest.raises(SkillManagementServiceError) as exc_info, ): - service._draft_payload_from_zip( - tenant_id=TENANT, - user_id=USER, - archive_bytes=package.getvalue(), - ) + service._read_skill_archive(archive_bytes=package.getvalue()) - assert exc_info.value.code == "skill_too_large" + assert (exc_info.value.code, exc_info.value.message) == ("skill_too_large", "skill is over 1 MiB uncompressed") def test_publish_and_export_include_binary_tool_files() -> None: @@ -3202,7 +3207,7 @@ def test_build_assistant_attachment_context_includes_extractable_pdf_text() -> N assert "Binary attachment available" not in context -def _zip_payload(files: dict[str, str]) -> bytes: +def _zip_payload(files: Mapping[str, str | bytes]) -> bytes: buffer = io.BytesIO() with zipfile.ZipFile(buffer, "w") as archive: for path, content in files.items(): @@ -3740,5 +3745,399 @@ def test_archive_limits_accept_high_compression_ratio_within_size_budget(monkeyp with zipfile.ZipFile(io.BytesIO(output.getvalue())) as archive: assert SkillManagementService._validate_archive_limits(archive)[0].file_size == 1024 * 1024 apply_config_overrides(monkeypatch, SKILL_PACKAGE_MAX_UNCOMPRESSED_BYTES=1024) - with pytest.raises(SkillManagementServiceError, match="uncompressed size limit"): + with pytest.raises(SkillManagementServiceError, match="^skill is over 1024 bytes uncompressed$"): SkillManagementService._validate_archive_limits(archive) + + +def test_archive_limits_name_the_file_count_limit(monkeypatch: pytest.MonkeyPatch) -> None: + apply_config_overrides(monkeypatch, SKILL_PACKAGE_MAX_ENTRIES=2) + content = _zip_payload({"SKILL.md": _skill_md(), "a.md": "a", "b.md": "b"}) + with zipfile.ZipFile(io.BytesIO(content)) as archive: + with pytest.raises(SkillManagementServiceError) as exc_info: + SkillManagementService._validate_archive_limits(archive) + + assert (exc_info.value.code, exc_info.value.message) == ("too_many_files", "skill has more than 2 files") + + +OTHER_TENANT = "99999999-9999-9999-9999-999999999999" +_LOGO = b"\x89PNG\r\n\x1a\n\x00logo" +_CHART = b"\x89PNG\r\n\x1a\n\x00chart" +_SAME_SIZE_AS_LOGO = b"\x89PNG\r\n\x1a\n\x00fake" +_NAME_DETAILS = {"path": "SKILL.md", "field": "name", "line": 2} +_DESCRIPTION_DETAILS = {**_NAME_DETAILS, "field": "description"} + + +class _ForcedFailureError(Exception): + pass + + +@pytest.fixture +def tool_files(monkeypatch: pytest.MonkeyPatch) -> _FakeToolFileManager: + manager = _FakeToolFileManager() + + def delete_object(key: str) -> None: + if key not in manager.stored: + raise FileNotFoundError(key) + del manager.stored[key] + + monkeypatch.setattr("services.skill_management_service.storage.delete", delete_object) + monkeypatch.setattr("services.skill_management_service.storage.load_once", manager.stored.__getitem__) + return manager + + +@pytest.fixture +def service(tool_files: _FakeToolFileManager) -> SkillManagementService: + return SkillManagementService(tool_file_manager=tool_files) + + +def _before[**P, R](hook: Callable[[], object], call: Callable[P, R]) -> Callable[P, R]: + def wrapped(*args: P.args, **kwargs: P.kwargs) -> R: + hook() + return call(*args, **kwargs) + + return wrapped + + +def _service_error(call: Callable[[], object]) -> SkillManagementServiceError: + with pytest.raises(SkillManagementServiceError) as exc_info: + call() + return exc_info.value + + +def _new_skill(service: SkillManagementService, name: str = "expense-sop") -> str: + return service.create_skill(tenant_id=TENANT, user_id=USER, payload=SkillCreatePayload(name=name))["id"] + + +def _package(binaries: Mapping[str, bytes], *, name: str = "expense-sop", body: str = "# Expenses") -> bytes: + return _zip_payload({"SKILL.md": _skill_md(name=name, description="Expenses", body=body), **binaries}) + + +def _import_package(service: SkillManagementService, content: bytes) -> str: + payload = SkillImportPayload(content=content, filename="expense-sop.zip") + return service.import_skill(tenant_id=TENANT, user_id=USER, payload=payload)["id"] + + +def _overwrite_package( + service: SkillManagementService, skill_id: str, content: bytes, *, expected_updated_at: int | None = None +) -> Mapping[str, object]: + payload = SkillOverwritePayload(content=content, expected_updated_at=expected_updated_at) + return service.overwrite_skill(tenant_id=TENANT, user_id=USER, skill_id=skill_id, payload=payload) + + +def _tool_file_ids() -> set[str]: + with session_factory.create_session() as session: + return set(session.scalars(select(ToolFile.id))) + + +def _tool_file_key(tool_file_id: str) -> str: + with session_factory.create_session() as session: + key = session.scalar(select(ToolFile.file_key).where(ToolFile.id == tool_file_id)) + assert key is not None + return key + + +def _draft_paths(skill_id: str) -> list[str]: + with session_factory.create_session() as session: + return sorted(session.scalars(select(SkillDraftFile.path).where(SkillDraftFile.skill_id == skill_id))) + + +def _draft_tool_file_ids(skill_id: str) -> dict[str, str]: + with session_factory.create_session() as session: + files = session.scalars(select(SkillDraftFile).where(SkillDraftFile.skill_id == skill_id)) + return {file.path: file.tool_file_id for file in files if file.tool_file_id is not None} + + +def _skill_with_logo(service: SkillManagementService) -> tuple[str, str]: + skill_id = _new_skill(service) + _overwrite_package(service, skill_id, _package({"assets/logo.png": _LOGO})) + return skill_id, _draft_tool_file_ids(skill_id)["assets/logo.png"] + + +def test_export_version_archive_serves_only_the_skills_own_version( + service: SkillManagementService, tool_files: _FakeToolFileManager +) -> None: + skill_id = _import_package(service, _package({})) + other_id = _import_package(service, _package({}, name="other-sop")) + publish = SkillPublishPayload() + version = service.publish_skill(tenant_id=TENANT, user_id=USER, skill_id=skill_id, payload=publish) + other_version = service.publish_skill(tenant_id=TENANT, user_id=USER, skill_id=other_id, payload=publish) + skill_zip, _other_zip = tool_files.stored.values() + + archive = service.export_version_archive(tenant_id=TENANT, skill_id=skill_id, version_id=version["id"]) + other_skills_version = _service_error( + lambda: service.export_version_archive(tenant_id=TENANT, skill_id=skill_id, version_id=other_version["id"]) + ) + other_workspace = _service_error( + lambda: service.export_version_archive(tenant_id=OTHER_TENANT, skill_id=skill_id, version_id=version["id"]) + ) + + assert archive.payload == skill_zip + assert (archive.filename, archive.mime_type) == ("expense-sop-v1.zip", "application/zip") + assert other_skills_version.code == SkillServiceErrorCode.SKILL_VERSION_NOT_FOUND + assert other_skills_version.status_code == 404 + assert other_workspace.code == SkillServiceErrorCode.SKILL_NOT_FOUND + + +@pytest.mark.parametrize( + ("extra", "paths"), + [ + ({".DS_Store": "junk", "__MACOSX/._SKILL.md": "junk", "scripts/Thumbs.db": "junk"}, ["SKILL.md"]), + ({"x.DS_Store": "kept", ".ds_store": "kept"}, [".ds_store", "SKILL.md", "x.DS_Store"]), + ], + ids=["os-junk-dropped", "lookalike-names-kept"], +) +def test_import_drops_os_junk_only(service: SkillManagementService, extra: dict[str, str], paths: list[str]) -> None: + skill_id = _import_package(service, _zip_payload({"SKILL.md": _skill_md(name="expense-sop"), **extra})) + assert [item["path"] for item in service.get_skill(tenant_id=TENANT, skill_id=skill_id)["files"]] == paths + + +def test_import_refused_for_a_taken_name_or_a_full_workspace_stores_no_files( + service: SkillManagementService, tool_files: _FakeToolFileManager, monkeypatch: pytest.MonkeyPatch +) -> None: + existing_id = _new_skill(service) + package = _package({"assets/logo.png": _LOGO}) + + at_pre_check = _service_error(lambda: _import_package(service, package)) + monkeypatch.setattr(service, "_reject_taken_skill_name", lambda _session, **_kwargs: None) + at_commit = _service_error(lambda: _import_package(service, package)) + monkeypatch.setattr("services.skill_management_service._MAX_SKILLS_PER_WORKSPACE", 1) + workspace_full = _service_error(lambda: _import_package(service, _package({"x.png": _LOGO}, name="other-sop"))) + + for error in (at_pre_check, at_commit): + assert (error.code, error.message) == (SkillServiceErrorCode.SKILL_NAME_CONFLICT, "skill name already exists") + assert at_pre_check.details == {SkillErrorDetail.SKILL_ID: existing_id} + assert at_commit.details == {SkillErrorDetail.NAME: "expense-sop"} + assert workspace_full.code == "skill_limit_exceeded" + assert (_tool_file_ids(), tool_files.stored) == (set(), {}) + + +def test_import_failure_after_upload_deletes_its_files_and_keeps_its_error_if_cleanup_fails( + service: SkillManagementService, tool_files: _FakeToolFileManager, monkeypatch: pytest.MonkeyPatch +) -> None: + def fail(**_kwargs: object) -> None: + assert tool_files.stored, "binaries are stored before the forced failure" + raise _ForcedFailureError + + def broken_delete(key: str) -> None: + raise OSError(key) + + monkeypatch.setattr(service, "_build_draft_rows_from_tree", fail) + with pytest.raises(_ForcedFailureError): + _import_package(service, _package({"assets/logo.png": _LOGO, "assets/chart.png": _CHART})) + assert (_tool_file_ids(), tool_files.stored) == (set(), {}) + with session_factory.create_session() as session: + assert session.scalar(select(func.count()).select_from(Skill)) == 0 + + monkeypatch.setattr("services.skill_management_service.storage.delete", broken_delete) + with pytest.raises(_ForcedFailureError): + _import_package(service, _package({"assets/logo.png": _LOGO})) + + +def test_import_uploads_while_no_transaction_is_open( + tool_files: _FakeToolFileManager, monkeypatch: pytest.MonkeyPatch +) -> None: + in_transaction_at_upload: list[bool] = [] + with session_factory.create_session() as session: + record = _before( + lambda: in_transaction_at_upload.append(session.in_transaction()), tool_files.create_file_by_raw + ) + monkeypatch.setattr(tool_files, "create_file_by_raw", record) + service = SkillManagementService(tool_file_manager=tool_files, session=session) + _import_package(service, _package({"assets/logo.png": _LOGO})) + assert in_transaction_at_upload == [False] + + +def test_overwrite_replaces_draft_and_syncs_metadata(service: SkillManagementService) -> None: + skill_id = _import_package(service, _package({"old.md": b"old"})) + before = service.update_metadata( + tenant_id=TENANT, user_id=USER, skill_id=skill_id, payload=SkillMetadataPayload(icon="🔥", tags=["finance"]) + ) + skill_md = "---\nname: expense-sop\ndescription: New description\nmetadata:\n display-name: New Display\n---\n" + + result = _overwrite_package(service, skill_id, _zip_payload({"SKILL.md": skill_md, "new.md": "new"})) + + assert (result["name"], result["description"]) == ("expense-sop", "New description") + assert (result["display_name"], result["icon"]) == ("New Display", "🔥") + assert result["tags"] == before["tags"] + assert result["tags"] + assert _draft_paths(skill_id) == ["SKILL.md", "new.md"] + + +def test_overwrite_without_a_display_name_takes_the_one_derived_from_the_name(service: SkillManagementService) -> None: + payload = SkillCreatePayload(name="expense-sop", display_name="Console Title") + created = service.create_skill(tenant_id=TENANT, user_id=USER, payload=payload) + + result = _overwrite_package(service, created["id"], _package({})) + + assert created["display_name"] == "Console Title" + expected = SkillManagementService._display_name_from_frontmatter(metadata={}, name="expense-sop") + assert result["display_name"] == expected == "Expense Sop" + assert service.get_skill(tenant_id=TENANT, skill_id=created["id"])["display_name"] == expected + + +def _import_into(service: SkillManagementService, _skill_id: str, content: bytes) -> None: + _import_package(service, content) + + +_NO_NAME = "---\ndescription: Expenses\n---\n" +_BAD_NAME = "---\nname: Expense SOP\ndescription: Expenses\n---\n" +_NO_DESCRIPTION = "---\nname: expense-sop\n---\n" + + +@pytest.mark.parametrize( + ("send", "skill_md", "code", "details"), + [ + (_import_into, _NO_NAME, SkillServiceErrorCode.MISSING_SKILL_NAME, _NAME_DETAILS), + (_import_into, _BAD_NAME, SkillServiceErrorCode.INVALID_SKILL_NAME, _NAME_DETAILS), + (_overwrite_package, _NO_NAME, SkillServiceErrorCode.MISSING_SKILL_NAME, _NAME_DETAILS), + (_overwrite_package, _BAD_NAME, SkillServiceErrorCode.INVALID_SKILL_NAME, _NAME_DETAILS), + (_overwrite_package, _NO_DESCRIPTION, SkillServiceErrorCode.MISSING_SKILL_DESCRIPTION, _DESCRIPTION_DETAILS), + ( + _overwrite_package, + _skill_md("other-sop"), + SkillServiceErrorCode.SKILL_NAME_MISMATCH, + {"expected": "expense-sop", "got": "other-sop"}, + ), + ], + ids=["import-no-name", "import-bad-name", "overwrite-no-name", "overwrite-bad-name", "no-description", "mismatch"], +) +def test_a_rejected_package_leaves_the_draft_and_storage_unchanged( + service: SkillManagementService, + tool_files: _FakeToolFileManager, + send: Callable[[SkillManagementService, str, bytes], object], + skill_md: str, + code: str, + details: dict[str, object], +) -> None: + skill_id = _new_skill(service) + paths_before = _draft_paths(skill_id) + + error = _service_error(lambda: send(service, skill_id, _zip_payload({"SKILL.md": skill_md, "x.png": _LOGO}))) + + assert (error.code, error.status_code, error.details) == (code, 400, details) + assert _draft_paths(skill_id) == paths_before + assert service.get_skill(tenant_id=TENANT, skill_id=skill_id)["name"] == "expense-sop" + assert (_tool_file_ids(), tool_files.stored) == (set(), {}) + + +def test_overwrite_refuses_a_stale_draft_hash_an_unknown_skill_and_an_oversized_archive( + service: SkillManagementService, monkeypatch: pytest.MonkeyPatch +) -> None: + skill_id = _new_skill(service) + + stale = _service_error(lambda: _overwrite_package(service, skill_id, b"never-parsed", expected_updated_at=1)) + unknown = _service_error(lambda: _overwrite_package(service, str(uuid4()), _package({}))) + apply_config_overrides(monkeypatch, UPLOAD_SKILL_FILE_SIZE_LIMIT=0) + oversized = _service_error(lambda: _overwrite_package(service, skill_id, b"not-empty")) + + assert (stale.code, stale.status_code) == (SkillServiceErrorCode.SKILL_CONFLICT, 409) + assert (unknown.code, unknown.status_code) == (SkillServiceErrorCode.SKILL_NOT_FOUND, 404) + assert oversized.code == SkillServiceErrorCode.ARCHIVE_TOO_LARGE + assert oversized.message == "skill archive is over 0 MiB" + assert _tool_file_ids() == set() + + +def test_overwrite_and_restore_reuse_a_matching_binary_at_any_path_loading_it_once( + service: SkillManagementService, tool_files: _FakeToolFileManager, monkeypatch: pytest.MonkeyPatch +) -> None: + skill_id, logo_id = _skill_with_logo(service) + version = service.publish_skill(tenant_id=TENANT, user_id=USER, skill_id=skill_id, payload=SkillPublishPayload()) + ids_before = _tool_file_ids() + loaded: list[str] = [] + + def load_once(key: str) -> bytes: + loaded.append(key) + return tool_files.stored[key] + + monkeypatch.setattr("services.skill_management_service.storage.load_once", load_once) + _overwrite_package(service, skill_id, _package({"assets/logo.png": _LOGO, "images/logo.png": _LOGO}, body="# New")) + + assert _draft_tool_file_ids(skill_id) == {"assets/logo.png": logo_id, "images/logo.png": logo_id} + assert loaded == [_tool_file_key(logo_id)] + assert any( + "# New" in (file["content"] or "") for file in service.get_skill(tenant_id=TENANT, skill_id=skill_id)["files"] + ) + + restore = SkillRestorePayload(version_id=version["id"]) + restored = service.restore_version(tenant_id=TENANT, user_id=USER, skill_id=skill_id, payload=restore) + + assert _draft_tool_file_ids(skill_id) == {"assets/logo.png": logo_id} + assert any("# Expenses" in (file["content"] or "") for file in restored["files"]) + assert _tool_file_ids() == ids_before + + +def test_overwrite_stores_a_new_file_for_changed_bytes_or_another_skills_binary( + service: SkillManagementService, +) -> None: + skill_id, logo_id = _skill_with_logo(service) + other_id = _new_skill(service, "other-sop") + + _overwrite_package(service, skill_id, _package({"assets/logo.png": _CHART})) + _overwrite_package(service, other_id, _package({"assets/logo.png": _LOGO}, name="other-sop")) + + chart_id = _draft_tool_file_ids(skill_id)["assets/logo.png"] + other_logo_id = _draft_tool_file_ids(other_id)["assets/logo.png"] + assert len({logo_id, chart_id, other_logo_id}) == 3 + assert _tool_file_ids() == {logo_id, chart_id, other_logo_id} + + +def test_overwrite_failure_after_upload_keeps_reused_files( + service: SkillManagementService, tool_files: _FakeToolFileManager, monkeypatch: pytest.MonkeyPatch +) -> None: + skill_id, logo_id = _skill_with_logo(service) + stored_before = dict(tool_files.stored) + + def fail(_session: Session, **_kwargs: object) -> None: + assert len(tool_files.stored) > len(stored_before), "the new binary is stored before the forced failure" + raise _ForcedFailureError + + monkeypatch.setattr(service, "_replace_draft_rows", fail) + with pytest.raises(_ForcedFailureError): + _overwrite_package(service, skill_id, _package({"assets/logo.png": _LOGO, "assets/chart.png": _CHART})) + + assert (_tool_file_ids(), tool_files.stored) == ({logo_id}, stored_before) + assert _draft_tool_file_ids(skill_id) == {"assets/logo.png": logo_id} + + +def _stored_tool_file(manager: _FakeToolFileManager, payload: bytes, *, tenant_id: str = TENANT) -> ToolFile: + return manager.create_file_by_raw( + user_id=USER, tenant_id=tenant_id, conversation_id=None, file_binary=payload, mimetype="image/png" + ) + + +def _unreadable_logo(manager: _FakeToolFileManager) -> str: + tool_file = _stored_tool_file(manager, _LOGO) + del manager.stored[tool_file.file_key] + return tool_file.id + + +@pytest.mark.parametrize( + "claimed_file", + [ + pytest.param(lambda manager: _stored_tool_file(manager, _SAME_SIZE_AS_LOGO).id, id="bytes-differ"), + pytest.param(lambda _manager: str(uuid4()), id="file-missing"), + pytest.param(lambda manager: _stored_tool_file(manager, _LOGO, tenant_id=OTHER_TENANT).id, id="other-tenant"), + pytest.param(_unreadable_logo, id="object-unreadable"), + ], +) +def test_overwrite_reuses_only_a_file_whose_stored_bytes_match_the_draft_claim( + service: SkillManagementService, + tool_files: _FakeToolFileManager, + claimed_file: Callable[[_FakeToolFileManager], str], +) -> None: + skill_id, logo_id = _skill_with_logo(service) + claimed_id = claimed_file(tool_files) + # The logo row keeps claiming the logo's hash and size, but now names the claimed file. + with session_factory.create_session() as session, session.begin(): + session.execute( + update(SkillDraftFile).where(SkillDraftFile.tool_file_id == logo_id).values(tool_file_id=claimed_id) + ) + ids_before, stored_before = _tool_file_ids(), dict(tool_files.stored) + + _overwrite_package(service, skill_id, _package({"assets/logo.png": _LOGO})) + + new_logo_id = _draft_tool_file_ids(skill_id)["assets/logo.png"] + assert new_logo_id not in {claimed_id, logo_id} + assert tool_files.stored[_tool_file_key(new_logo_id)] == _LOGO + assert ids_before <= _tool_file_ids() + assert stored_before.items() <= tool_files.stored.items() diff --git a/cli/test/fixtures/catalog.json b/cli/test/fixtures/catalog.json index 85e3badda2c..4d0f8f29e8a 100644 --- a/cli/test/fixtures/catalog.json +++ b/cli/test/fixtures/catalog.json @@ -1835,6 +1835,106 @@ "method": "POST", "path": "/openapi/v1/workspaces/{workspace_id}/tool-providers/{provider}/credentials", "summary": "Save an API-key credential for a tool provider; the provider checks it first" + }, + "get.skill": { + "bind": { + "keyword": "query", + "limit": "query", + "page": "query", + "tag": "query", + "workspace_id": "path" + }, + "deprecated": false, + "input": { + "type": "object", + "properties": { "keyword": {}, "limit": {}, "page": {}, "tag": {}, "workspace_id": {} }, + "required": ["workspace_id"] + }, + "internal": false, + "kind": "list", + "method": "GET", + "path": "/openapi/v1/workspaces/{workspace_id}/skills", + "summary": "Skills in the workspace, newest edit first" + }, + "describe.skill": { + "bind": { "skill_id": "path", "workspace_id": "path" }, + "deprecated": false, + "input": { + "type": "object", + "properties": { "skill_id": {}, "workspace_id": {} }, + "required": ["skill_id", "workspace_id"] + }, + "internal": false, + "kind": "object", + "method": "GET", + "path": "/openapi/v1/workspaces/{workspace_id}/skills/{skill_id}", + "summary": "One skill with its draft hash and latest published version" + }, + "get.skill.version": { + "bind": { "limit": "query", "page": "query", "skill_id": "path", "workspace_id": "path" }, + "deprecated": false, + "input": { + "type": "object", + "properties": { "limit": {}, "page": {}, "skill_id": {}, "workspace_id": {} }, + "required": ["skill_id", "workspace_id"] + }, + "internal": false, + "kind": "list", + "method": "GET", + "path": "/openapi/v1/workspaces/{workspace_id}/skills/{skill_id}/versions", + "summary": "Published versions of a skill, newest first" + }, + "export.skill": { + "bind": { "skill_id": "path", "version_id": "query", "workspace_id": "path" }, + "deprecated": false, + "input": { + "type": "object", + "properties": { "skill_id": {}, "version_id": {}, "workspace_id": {} }, + "required": ["skill_id", "workspace_id"] + }, + "internal": false, + "kind": "file", + "method": "GET", + "path": "/openapi/v1/workspaces/{workspace_id}/skills/{skill_id}/archive", + "summary": "Download a skill as a zip: its draft, or one published version" + }, + "import.skill": { + "bind": { "draft_hash": "body", "files": "file", "skill_id": "body", "workspace_id": "path" }, + "deprecated": false, + "input": { + "type": "object", + "properties": { "draft_hash": {}, "files": {}, "skill_id": {}, "workspace_id": {} }, + "required": ["workspace_id", "files"] + }, + "internal": false, + "kind": "object", + "method": "POST", + "path": "/openapi/v1/workspaces/{workspace_id}/skills/imports", + "summary": "Create a skill from a folder, or overwrite a skill's draft with one" + }, + "publish.skill": { + "bind": { + "publish_note": "body", + "skill_id": "path", + "version_name": "body", + "workspace_id": "path" + }, + "deprecated": false, + "input": { + "type": "object", + "properties": { + "publish_note": {}, + "skill_id": {}, + "version_name": {}, + "workspace_id": {} + }, + "required": ["skill_id", "workspace_id"] + }, + "internal": false, + "kind": "object", + "method": "POST", + "path": "/openapi/v1/workspaces/{workspace_id}/skills/{skill_id}:publish", + "summary": "Publish the draft as a new version" } } } diff --git a/packages/contracts/generated/api/openapi/orpc.gen.ts b/packages/contracts/generated/api/openapi/orpc.gen.ts index b6f5b92fc30..18fa0582125 100644 --- a/packages/contracts/generated/api/openapi/orpc.gen.ts +++ b/packages/contracts/generated/api/openapi/orpc.gen.ts @@ -128,6 +128,17 @@ import { zGetWorkspacesByWorkspaceIdPluginTasksByTaskIdPath, zGetWorkspacesByWorkspaceIdPluginTasksByTaskIdResponse, zGetWorkspacesByWorkspaceIdResponse, + zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchivePath, + zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveQuery, + zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveResponse, + zGetWorkspacesByWorkspaceIdSkillsBySkillIdPath, + zGetWorkspacesByWorkspaceIdSkillsBySkillIdResponse, + zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsPath, + zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsQuery, + zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsResponse, + zGetWorkspacesByWorkspaceIdSkillsPath, + zGetWorkspacesByWorkspaceIdSkillsQuery, + zGetWorkspacesByWorkspaceIdSkillsResponse, zGetWorkspacesByWorkspaceIdToolProvidersByProviderPath, zGetWorkspacesByWorkspaceIdToolProvidersByProviderResponse, zGetWorkspacesByWorkspaceIdToolsPath, @@ -269,6 +280,12 @@ import { zPostWorkspacesByWorkspaceIdPluginsInstallBody, zPostWorkspacesByWorkspaceIdPluginsInstallPath, zPostWorkspacesByWorkspaceIdPluginsInstallResponse, + zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishBody, + zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishPath, + zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishResponse, + zPostWorkspacesByWorkspaceIdSkillsImportsBody, + zPostWorkspacesByWorkspaceIdSkillsImportsPath, + zPostWorkspacesByWorkspaceIdSkillsImportsResponse, zPostWorkspacesByWorkspaceIdToolProvidersByProviderCredentialsBody, zPostWorkspacesByWorkspaceIdToolProvidersByProviderCredentialsPath, zPostWorkspacesByWorkspaceIdToolProvidersByProviderCredentialsResponse, @@ -2310,6 +2327,135 @@ export const plugins2 = { install, } +/** + * Import never publishes + * + * ``draft_hash`` is the draft's ``updated_at`` in whole seconds, + * so an edit made in the same second as the export is not detected. + */ +export const post31 = oc + .route({ + description: + "``draft_hash`` is the draft's ``updated_at`` in whole seconds,\nso an edit made in the same second as the export is not detected.", + inputStructure: 'detailed', + method: 'POST', + operationId: 'postWorkspacesByWorkspaceIdSkillsImports', + path: '/workspaces/{workspace_id}/skills/imports', + summary: 'Import never publishes', + tags: ['openapi'], + }) + .input( + z.object({ + body: zPostWorkspacesByWorkspaceIdSkillsImportsBody, + params: zPostWorkspacesByWorkspaceIdSkillsImportsPath, + }), + ) + .output(zPostWorkspacesByWorkspaceIdSkillsImportsResponse) + +export const imports2 = { + post: post31, +} + +export const get52 = oc + .route({ + inputStructure: 'detailed', + method: 'GET', + operationId: 'getWorkspacesByWorkspaceIdSkillsBySkillIdArchive', + path: '/workspaces/{workspace_id}/skills/{skill_id}/archive', + tags: ['openapi'], + }) + .input( + z.object({ + params: zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchivePath, + query: zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveQuery.optional(), + }), + ) + .output(zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveResponse) + +export const archive = { + get: get52, +} + +export const get53 = oc + .route({ + inputStructure: 'detailed', + method: 'GET', + operationId: 'getWorkspacesByWorkspaceIdSkillsBySkillIdVersions', + path: '/workspaces/{workspace_id}/skills/{skill_id}/versions', + tags: ['openapi'], + }) + .input( + z.object({ + params: zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsPath, + query: zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsQuery.optional(), + }), + ) + .output(zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsResponse) + +export const versions2 = { + get: get53, +} + +export const post32 = oc + .route({ + inputStructure: 'detailed', + method: 'POST', + operationId: 'postWorkspacesByWorkspaceIdSkillsBySkillIdPublish', + path: '/workspaces/{workspace_id}/skills/{skill_id}:publish', + tags: ['openapi'], + }) + .input( + z.object({ + body: zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishBody, + params: zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishPath, + }), + ) + .output(zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishResponse) + +export const publish2 = { + post: post32, +} + +export const get54 = oc + .route({ + inputStructure: 'detailed', + method: 'GET', + operationId: 'getWorkspacesByWorkspaceIdSkillsBySkillId', + path: '/workspaces/{workspace_id}/skills/{skill_id}', + tags: ['openapi'], + }) + .input(z.object({ params: zGetWorkspacesByWorkspaceIdSkillsBySkillIdPath })) + .output(zGetWorkspacesByWorkspaceIdSkillsBySkillIdResponse) + +export const bySkillId = { + get: get54, + archive, + versions: versions2, + publish: publish2, +} + +export const get55 = oc + .route({ + inputStructure: 'detailed', + method: 'GET', + operationId: 'getWorkspacesByWorkspaceIdSkills', + path: '/workspaces/{workspace_id}/skills', + tags: ['openapi'], + }) + .input( + z.object({ + params: zGetWorkspacesByWorkspaceIdSkillsPath, + query: zGetWorkspacesByWorkspaceIdSkillsQuery.optional(), + }), + ) + .output(zGetWorkspacesByWorkspaceIdSkillsResponse) + +export const skills = { + get: get55, + imports: imports2, + bySkillId, +} + export const patch18 = oc .route({ inputStructure: 'detailed', @@ -2330,7 +2476,7 @@ export const byCredentialId3 = { patch: patch18, } -export const post31 = oc +export const post33 = oc .route({ inputStructure: 'detailed', method: 'POST', @@ -2348,11 +2494,11 @@ export const post31 = oc .output(zPostWorkspacesByWorkspaceIdToolProvidersByProviderCredentialsResponse) export const credentials3 = { - post: post31, + post: post33, byCredentialId: byCredentialId3, } -export const get52 = oc +export const get56 = oc .route({ inputStructure: 'detailed', method: 'GET', @@ -2364,7 +2510,7 @@ export const get52 = oc .output(zGetWorkspacesByWorkspaceIdToolProvidersByProviderResponse) export const byProvider2 = { - get: get52, + get: get56, credentials: credentials3, } @@ -2372,7 +2518,7 @@ export const toolProviders = { byProvider: byProvider2, } -export const get53 = oc +export const get57 = oc .route({ inputStructure: 'detailed', method: 'GET', @@ -2389,10 +2535,10 @@ export const get53 = oc .output(zGetWorkspacesByWorkspaceIdToolsResponse) export const tools = { - get: get53, + get: get57, } -export const get54 = oc +export const get58 = oc .route({ inputStructure: 'detailed', method: 'GET', @@ -2404,7 +2550,7 @@ export const get54 = oc .output(zGetWorkspacesByWorkspaceIdResponse) export const byWorkspaceId = { - get: get54, + get: get58, apps: apps2, knowledgeBases, knowledgeFs, @@ -2414,11 +2560,12 @@ export const byWorkspaceId = { models: models2, pluginTasks, plugins: plugins2, + skills, toolProviders, tools, } -export const get55 = oc +export const get59 = oc .route({ inputStructure: 'detailed', method: 'GET', @@ -2430,7 +2577,7 @@ export const get55 = oc .output(zGetWorkspacesResponse) export const workspaces = { - get: get55, + get: get59, byWorkspaceId, } diff --git a/packages/contracts/generated/api/openapi/types.gen.ts b/packages/contracts/generated/api/openapi/types.gen.ts index 849a517f96f..85fd84238ec 100644 --- a/packages/contracts/generated/api/openapi/types.gen.ts +++ b/packages/contracts/generated/api/openapi/types.gen.ts @@ -257,6 +257,8 @@ export type AppSettingsInfo = { name: string } +export type BinaryFileResponse = Blob | File + export type ChatAppInfo = { description?: string | null icon?: string | null @@ -1097,6 +1099,13 @@ export type OpenApiErrorCode = | 'secret_mask_not_secret' | 'secret_mask_unknown_id' | 'service_api_disabled' + | 'skill_name_conflict' + | 'skill_name_mismatch' + | 'skill_not_found' + | 'skill_package_invalid' + | 'skill_request_rejected' + | 'skill_version_not_found' + | 'skills_disabled' | 'too_many_files' | 'too_many_requests' | 'trigger_workflow_service_mode_unavailable' @@ -1351,6 +1360,105 @@ export type SimpleResultResponse = { result: string } +export type SkillDescribeResponse = { + description: string + display_name: string + draft_hash: string + hints?: Array + icon: string | null + id: string + latest_published_version_id: string | null + latest_version: SkillVersionRow | null + name: string + tags: Array + updated_at: number +} + +export type SkillExportQuery = { + version_id?: string | null +} + +export type SkillImportPayload = { + draft_hash?: string | null + files: { + [key: string]: Blob | File | Array + } + skill_id?: string | null +} + +export type SkillImportResponse = { + draft_hash: string + file_count: number + hints?: Array + id: string + name: string +} + +export type SkillListQuery = { + keyword?: string | null + limit?: number + page?: number + tag?: Array | null +} + +export type SkillListResponse = { + data: Array + has_more: boolean + hints?: Array + limit: number + page: number + total: number +} + +export type SkillPublishPayload = { + publish_note?: string + version_name?: string | null +} + +export type SkillPublishResponse = { + created_at: number + created_by: string | null + hints?: Array + id: string + publish_note: string + version_name: string | null + version_number: number +} + +export type SkillRow = { + description: string + display_name: string + icon: string | null + id: string + latest_published_version_id: string | null + name: string + tags: Array + updated_at: number +} + +export type SkillVersionListQuery = { + limit?: number + page?: number +} + +export type SkillVersionListResponse = { + data: Array + has_more: boolean + hints?: Array + limit: number + page: number + total: number +} + +export type SkillVersionRow = { + created_at: number + created_by: string | null + id: string + publish_note: string + version_name: string | null + version_number: number +} + export type SubjectType = 'account' | 'external_sso' | 'resource_access' export type SupportedAppType = 'advanced-chat' | 'agent-chat' | 'chat' | 'completion' | 'workflow' @@ -4414,6 +4522,164 @@ export type PostWorkspacesByWorkspaceIdPluginsInstallResponses = { export type PostWorkspacesByWorkspaceIdPluginsInstallResponse = PostWorkspacesByWorkspaceIdPluginsInstallResponses[keyof PostWorkspacesByWorkspaceIdPluginsInstallResponses] +export type GetWorkspacesByWorkspaceIdSkillsData = { + body?: never + path: { + workspace_id: string + } + query?: { + keyword?: string + limit?: number + page?: number + tag?: Array + } + url: '/workspaces/{workspace_id}/skills' +} + +export type GetWorkspacesByWorkspaceIdSkillsErrors = { + 422: ErrorBody + default: ErrorBody +} + +export type GetWorkspacesByWorkspaceIdSkillsError = + GetWorkspacesByWorkspaceIdSkillsErrors[keyof GetWorkspacesByWorkspaceIdSkillsErrors] + +export type GetWorkspacesByWorkspaceIdSkillsResponses = { + 200: SkillListResponse +} + +export type GetWorkspacesByWorkspaceIdSkillsResponse = + GetWorkspacesByWorkspaceIdSkillsResponses[keyof GetWorkspacesByWorkspaceIdSkillsResponses] + +export type PostWorkspacesByWorkspaceIdSkillsImportsData = { + body: SkillImportPayload + path: { + workspace_id: string + } + query?: never + url: '/workspaces/{workspace_id}/skills/imports' +} + +export type PostWorkspacesByWorkspaceIdSkillsImportsErrors = { + 422: ErrorBody + default: ErrorBody +} + +export type PostWorkspacesByWorkspaceIdSkillsImportsError = + PostWorkspacesByWorkspaceIdSkillsImportsErrors[keyof PostWorkspacesByWorkspaceIdSkillsImportsErrors] + +export type PostWorkspacesByWorkspaceIdSkillsImportsResponses = { + 200: SkillImportResponse + 201: SkillImportResponse +} + +export type PostWorkspacesByWorkspaceIdSkillsImportsResponse = + PostWorkspacesByWorkspaceIdSkillsImportsResponses[keyof PostWorkspacesByWorkspaceIdSkillsImportsResponses] + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdData = { + body?: never + path: { + skill_id: string + workspace_id: string + } + query?: never + url: '/workspaces/{workspace_id}/skills/{skill_id}' +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdErrors = { + default: ErrorBody +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdError = + GetWorkspacesByWorkspaceIdSkillsBySkillIdErrors[keyof GetWorkspacesByWorkspaceIdSkillsBySkillIdErrors] + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdResponses = { + 200: SkillDescribeResponse +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdResponse = + GetWorkspacesByWorkspaceIdSkillsBySkillIdResponses[keyof GetWorkspacesByWorkspaceIdSkillsBySkillIdResponses] + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveData = { + body?: never + path: { + skill_id: string + workspace_id: string + } + query?: { + version_id?: string + } + url: '/workspaces/{workspace_id}/skills/{skill_id}/archive' +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveErrors = { + 422: ErrorBody + default: ErrorBody +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveError = + GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveErrors[keyof GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveErrors] + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveResponses = { + 200: BinaryFileResponse +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveResponse = + GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveResponses[keyof GetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveResponses] + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsData = { + body?: never + path: { + skill_id: string + workspace_id: string + } + query?: { + limit?: number + page?: number + } + url: '/workspaces/{workspace_id}/skills/{skill_id}/versions' +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsErrors = { + 422: ErrorBody + default: ErrorBody +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsError = + GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsErrors[keyof GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsErrors] + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsResponses = { + 200: SkillVersionListResponse +} + +export type GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsResponse = + GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsResponses[keyof GetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsResponses] + +export type PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishData = { + body: SkillPublishPayload + path: { + skill_id: string + workspace_id: string + } + query?: never + url: '/workspaces/{workspace_id}/skills/{skill_id}:publish' +} + +export type PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishErrors = { + 422: ErrorBody + default: ErrorBody +} + +export type PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishError = + PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishErrors[keyof PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishErrors] + +export type PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishResponses = { + 200: SkillPublishResponse +} + +export type PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishResponse = + PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishResponses[keyof PostWorkspacesByWorkspaceIdSkillsBySkillIdPublishResponses] + export type GetWorkspacesByWorkspaceIdToolProvidersByProviderData = { body?: never path: { diff --git a/packages/contracts/generated/api/openapi/zod.gen.ts b/packages/contracts/generated/api/openapi/zod.gen.ts index 36b0c467ec1..e81fa693bf7 100644 --- a/packages/contracts/generated/api/openapi/zod.gen.ts +++ b/packages/contracts/generated/api/openapi/zod.gen.ts @@ -285,6 +285,13 @@ export const zAppSettingsInfo = z.object({ name: z.string(), }) +/** + * BinaryFileResponse + */ +export const zBinaryFileResponse = z.custom( + (value) => value instanceof Blob || value instanceof File, +) + /** * ChatAppInfo */ @@ -1467,6 +1474,13 @@ export const zOpenApiErrorCode = z.enum([ 'secret_mask_not_secret', 'secret_mask_unknown_id', 'service_api_disabled', + 'skill_name_conflict', + 'skill_name_mismatch', + 'skill_not_found', + 'skill_package_invalid', + 'skill_request_rejected', + 'skill_version_not_found', + 'skills_disabled', 'too_many_files', 'too_many_requests', 'trigger_workflow_service_mode_unavailable', @@ -1842,6 +1856,148 @@ export const zSimpleResultResponse = z.object({ result: z.string(), }) +/** + * SkillExportQuery + */ +export const zSkillExportQuery = z.object({ + version_id: z.uuid().nullish(), +}) + +/** + * SkillImportPayload + */ +export const zSkillImportPayload = z.object({ + draft_hash: z + .string() + .regex(/^[0-9]+$/) + .nullish(), + files: z.record( + z.string(), + z.union([ + z.custom((value) => value instanceof Blob || value instanceof File), + z.array(z.custom((value) => value instanceof Blob || value instanceof File)), + ]), + ), + skill_id: z.uuid().nullish(), +}) + +/** + * SkillImportResponse + */ +export const zSkillImportResponse = z.object({ + draft_hash: z.string(), + file_count: z.int(), + hints: z.array(zHint).optional(), + id: z.string(), + name: z.string(), +}) + +/** + * SkillListQuery + */ +export const zSkillListQuery = z.object({ + keyword: z.string().max(200).nullish(), + limit: z.int().gte(1).lte(100).optional().default(20), + page: z.int().gte(1).optional().default(1), + tag: z.array(z.string()).nullish(), +}) + +/** + * SkillPublishPayload + */ +export const zSkillPublishPayload = z.object({ + publish_note: z.string().max(1024).optional().default(''), + version_name: z.string().max(128).nullish(), +}) + +/** + * SkillPublishResponse + */ +export const zSkillPublishResponse = z.object({ + created_at: z.int(), + created_by: z.string().nullable(), + hints: z.array(zHint).optional(), + id: z.string(), + publish_note: z.string(), + version_name: z.string().nullable(), + version_number: z.int(), +}) + +/** + * SkillRow + */ +export const zSkillRow = z.object({ + description: z.string(), + display_name: z.string(), + icon: z.string().nullable(), + id: z.string(), + latest_published_version_id: z.string().nullable(), + name: z.string(), + tags: z.array(z.string()), + updated_at: z.int(), +}) + +/** + * SkillListResponse + */ +export const zSkillListResponse = z.object({ + data: z.array(zSkillRow), + has_more: z.boolean(), + hints: z.array(zHint).optional(), + limit: z.int(), + page: z.int(), + total: z.int(), +}) + +/** + * SkillVersionListQuery + */ +export const zSkillVersionListQuery = z.object({ + limit: z.int().gte(1).lte(100).optional().default(20), + page: z.int().gte(1).optional().default(1), +}) + +/** + * SkillVersionRow + */ +export const zSkillVersionRow = z.object({ + created_at: z.int(), + created_by: z.string().nullable(), + id: z.string(), + publish_note: z.string(), + version_name: z.string().nullable(), + version_number: z.int(), +}) + +/** + * SkillDescribeResponse + */ +export const zSkillDescribeResponse = z.object({ + description: z.string(), + display_name: z.string(), + draft_hash: z.string(), + hints: z.array(zHint).optional(), + icon: z.string().nullable(), + id: z.string(), + latest_published_version_id: z.string().nullable(), + latest_version: zSkillVersionRow.nullable(), + name: z.string(), + tags: z.array(z.string()), + updated_at: z.int(), +}) + +/** + * SkillVersionListResponse + */ +export const zSkillVersionListResponse = z.object({ + data: z.array(zSkillVersionRow), + has_more: z.boolean(), + hints: z.array(zHint).optional(), + limit: z.int(), + page: z.int(), + total: z.int(), +}) + /** * SubjectType */ @@ -3669,6 +3825,84 @@ export const zPostWorkspacesByWorkspaceIdPluginsInstallPath = z.object({ */ export const zPostWorkspacesByWorkspaceIdPluginsInstallResponse = zPluginTaskStartResponse +export const zGetWorkspacesByWorkspaceIdSkillsPath = z.object({ + workspace_id: z.string(), +}) + +export const zGetWorkspacesByWorkspaceIdSkillsQuery = z.object({ + keyword: z.string().max(200).optional(), + limit: z.int().gte(1).lte(100).optional().default(20), + page: z.int().gte(1).optional().default(1), + tag: z.array(z.string()).optional(), +}) + +/** + * Skills + */ +export const zGetWorkspacesByWorkspaceIdSkillsResponse = zSkillListResponse + +export const zPostWorkspacesByWorkspaceIdSkillsImportsBody = zSkillImportPayload + +export const zPostWorkspacesByWorkspaceIdSkillsImportsPath = z.object({ + workspace_id: z.string(), +}) + +/** + * Overwritten + */ +export const zPostWorkspacesByWorkspaceIdSkillsImportsResponse = zSkillImportResponse + +export const zGetWorkspacesByWorkspaceIdSkillsBySkillIdPath = z.object({ + skill_id: z.string(), + workspace_id: z.string(), +}) + +/** + * Skill + */ +export const zGetWorkspacesByWorkspaceIdSkillsBySkillIdResponse = zSkillDescribeResponse + +export const zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchivePath = z.object({ + skill_id: z.string(), + workspace_id: z.string(), +}) + +export const zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveQuery = z.object({ + version_id: z.uuid().optional(), +}) + +/** + * Zip archive + */ +export const zGetWorkspacesByWorkspaceIdSkillsBySkillIdArchiveResponse = zBinaryFileResponse + +export const zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsPath = z.object({ + skill_id: z.string(), + workspace_id: z.string(), +}) + +export const zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsQuery = z.object({ + limit: z.int().gte(1).lte(100).optional().default(20), + page: z.int().gte(1).optional().default(1), +}) + +/** + * Versions + */ +export const zGetWorkspacesByWorkspaceIdSkillsBySkillIdVersionsResponse = zSkillVersionListResponse + +export const zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishBody = zSkillPublishPayload + +export const zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishPath = z.object({ + skill_id: z.string(), + workspace_id: z.string(), +}) + +/** + * Published + */ +export const zPostWorkspacesByWorkspaceIdSkillsBySkillIdPublishResponse = zSkillPublishResponse + export const zGetWorkspacesByWorkspaceIdToolProvidersByProviderPath = z.object({ provider: z.string(), workspace_id: z.string(), diff --git a/packages/contracts/non-json-openapi-responses.md b/packages/contracts/non-json-openapi-responses.md index 01b123a5700..33a569820e2 100644 --- a/packages/contracts/non-json-openapi-responses.md +++ b/packages/contracts/non-json-openapi-responses.md @@ -16,6 +16,7 @@ This inventory records non-JSON runtime responses and their schema intent. Legac | console | POST | `/datasets/{dataset_id}/documents/download-zip` | `application/zip` attachment | `BinaryFileResponse` | | service | POST | `/datasets/{dataset_id}/documents/download-zip` | `application/zip` attachment | `BinaryFileResponse` | | service | GET | `/files/{file_id}/preview` | Original file MIME type, optionally attachment | `BinaryFileResponse` | +| openapi | GET | `/workspaces/{workspace_id}/skills/{skill_id}/archive` | `application/zip` attachment | `BinaryFileResponse` | | console | GET | `/workspaces/current/plugin/icon` | Plugin asset MIME type | `string (binary)` | | console | GET | `/workspaces/current/plugin/asset` | `application/octet-stream` | `string (binary)` | | console | GET | `/workspaces/current/skills/{skill_id}/files/content` | Skill file MIME type, optionally attachment | `string (binary)` | diff --git a/skills/difyctl/SKILL.md b/skills/difyctl/SKILL.md index 88b3de7cff2..9b5497a5c8f 100644 --- a/skills/difyctl/SKILL.md +++ b/skills/difyctl/SKILL.md @@ -32,3 +32,4 @@ Not logged in, or the server unreachable, gives the local commands plus one line - Read [`references/operations.md`](references/operations.md) before running any operation: flags, inputs, files, results, hints, errors and exit codes. - Read [`references/build-workflow.md`](references/build-workflow.md) when asked to create, change, test or publish a Workflow or Chatflow app. - Read [`references/plugins.md`](references/plugins.md) to find tools, models and knowledge bases before building, when a plugin, model, tool or credential is missing, or when the human asks to install or remove a plugin. +- Read [`references/workspace-skills.md`](references/workspace-skills.md) to list, read, create, change, roll back or publish a workspace skill. diff --git a/skills/difyctl/references/workspace-skills.md b/skills/difyctl/references/workspace-skills.md new file mode 100644 index 00000000000..4e662d7bf59 --- /dev/null +++ b/skills/difyctl/references/workspace-skills.md @@ -0,0 +1,50 @@ +# Workspace skills + +A workspace skill is a folder with a `SKILL.md` at its root, plus any files it uses. Skills have a draft and published versions. Import writes the draft. Agents use only what is published. + +## Find a skill + +1. `difyctl get skill` lists the skills in the workspace. +2. `difyctl describe skill --skill-id ` shows one skill. Keep its `draft_hash`. `latest_version` is null while the skill has never been published. +3. `difyctl get skill version --skill-id ` lists the published versions. + +## Create a skill + +1. Put the skill in a folder with `SKILL.md` at its root. `name` and `description` in its frontmatter are required. +2. `difyctl import skill --files ` +3. `difyctl publish skill --skill-id `. Import never publishes. Use the `skill_id` from the import result. + +A name already taken gives 409 `skill_name_conflict`. Its hint is an import with `--skill-id` set to the skill that has the name. Use it only if the human wants to replace that skill. If another import took the name a moment earlier, the server does not know that skill's id, so the hint is `difyctl get skill --keyword ` instead. Take the id from that list. + +## Change a skill + +1. `difyctl describe skill --skill-id ` and keep `draft_hash`. +2. `difyctl export skill --skill-id --output .zip` saves the draft as a zip. +3. Unzip into a new, empty folder: `unzip -q .zip -d `, or `python3 -m zipfile -e .zip `, or on Windows `tar -xf .zip -C `. +4. Edit the files in the folder. +5. `difyctl import skill --files --skill-id --draft-hash ` +6. `difyctl publish skill --skill-id ` + +Leaving out `--draft-hash` on an overwrite skips the conflict check, so the import replaces whatever the draft holds now. + +The folder replaces the whole draft. A file you leave out of the folder is deleted from the draft. + +If step 5 gives 409, someone changed or published the skill since you took the hash. Run steps 1 to 4 again and redo your edits on the new export. + +## Read what is live + +Export the published version, not the draft: `difyctl export skill --skill-id --version-id `. Take `` from `latest_version.id` in the `describe skill` result. + +## Roll back + +1. Pick the version with `difyctl get skill version --skill-id `. +2. Export it with `--version-id`, unzip it, and import the folder with `--skill-id` and the current `--draft-hash`. +3. Publish. + +## Rules + +- `name` in `SKILL.md` must equal the name of the skill you overwrite. A different name gives 400. +- Import takes a folder, never a zip. +- `.DS_Store`, `Thumbs.db`, `desktop.ini` and `__MACOSX` entries are dropped. +- `draft_hash` has one-second resolution. Two edits in the same second are not told apart. +- A folder over the server's file count or size limit gives 413 and the message names the limit. Don't assume the numbers; trim the folder to fit what the message says.