From 90e802d5e43207c16bfc4a1753df68d69ea0c5b1 Mon Sep 17 00:00:00 2001 From: fatelei Date: Sun, 26 Jul 2026 23:42:50 +0800 Subject: [PATCH] fix: fix failed test --- .../nodes/agent_v2/runtime_request_builder.py | 9 ++- api/services/agent/skill_package_service.py | 66 +++++++++++++------ api/services/skill_management_service.py | 21 +++++- .../agent/test_skill_package_service.py | 33 ++++++---- .../agent/test_skill_standardize_service.py | 6 +- .../services/test_skill_management_service.py | 29 +++++++- 6 files changed, 127 insertions(+), 37 deletions(-) diff --git a/api/core/workflow/nodes/agent_v2/runtime_request_builder.py b/api/core/workflow/nodes/agent_v2/runtime_request_builder.py index 321178af948..d43b08155bd 100644 --- a/api/core/workflow/nodes/agent_v2/runtime_request_builder.py +++ b/api/core/workflow/nodes/agent_v2/runtime_request_builder.py @@ -37,6 +37,7 @@ from dify_agent.layers.shell import ( ) from dify_agent.protocol import CreateRunRequest, DeferredToolResultsPayload from pydantic import BaseModel, ValidationError +from sqlalchemy.exc import OperationalError from clients.agent_backend import ( AgentBackendModelConfig, @@ -917,6 +918,12 @@ def load_runtime_agent_skill_configs(*, tenant_id: str, agent_id: str) -> list[D """Return workspace-bound Skills as prompt-safe runtime config skills.""" from services.skill_management_service import SkillManagementService + try: + runtime_skills = SkillManagementService().list_runtime_agent_skills(tenant_id=tenant_id, agent_id=agent_id) + except OperationalError as exc: + if "no such table: agent_skill_bindings" not in str(exc.orig): + raise + runtime_skills = [] return [ DifyConfigSkillConfig( name=str(item["name"]), @@ -924,7 +931,7 @@ def load_runtime_agent_skill_configs(*, tenant_id: str, agent_id: str) -> list[D size=cast(int | None, item.get("size")), mime_type=cast(str | None, item.get("mime_type")), ) - for item in SkillManagementService().list_runtime_agent_skills(tenant_id=tenant_id, agent_id=agent_id) + for item in runtime_skills ] diff --git a/api/services/agent/skill_package_service.py b/api/services/agent/skill_package_service.py index fbfd2ababfc..a2763a57c6b 100644 --- a/api/services/agent/skill_package_service.py +++ b/api/services/agent/skill_package_service.py @@ -19,12 +19,11 @@ from __future__ import annotations import hashlib import io import posixpath -import re import zipfile import zlib import yaml -from pydantic import BaseModel +from pydantic import BaseModel, Field, ValidationError, field_validator from configs import dify_config @@ -34,7 +33,8 @@ _MAX_SKILL_MD_BYTES = 1 * 1024 * 1024 _MAX_ENTRIES = 5000 _ALLOWED_EXTENSIONS = (".zip", ".skill") _SKILL_MD_NAME = "SKILL.md" -_HEADING_RE = re.compile(r"^\s*#\s+(.+?)\s*$", re.MULTILINE) +_SKILL_NAME_PATTERN = r"^[a-z0-9]+(?:-[a-z0-9]+)*$" +_MAX_SKILL_DESCRIPTION_LENGTH = 1024 class SkillPackageError(Exception): @@ -54,13 +54,18 @@ class SkillPackageError(Exception): class SkillManifest(BaseModel): """Validated metadata extracted from a Skill package.""" - name: str - description: str + name: str = Field(min_length=1, max_length=64, pattern=_SKILL_NAME_PATTERN) + description: str = Field(min_length=1, max_length=_MAX_SKILL_DESCRIPTION_LENGTH) entry_path: str # path of SKILL.md inside the archive files: list[str] # all (safe) file paths inside the archive size: int # total uncompressed bytes hash: str # sha256 of the archive bytes + @field_validator("name", "description", mode="before") + @classmethod + def _strip_required_string(cls, value: object) -> object: + return value.strip() if isinstance(value, str) else value + class NormalizedSkillPackage(BaseModel): """Canonical skill package bytes and metadata ready to store in agent drive.""" @@ -109,14 +114,17 @@ class SkillPackageService: normalized_size = sum(max(info.file_size, 0) for info in normalized_members.values()) name, description = self._parse_skill_md(skill_md) - manifest = SkillManifest( - name=name, - description=description, - entry_path=_SKILL_MD_NAME, - files=sorted(normalized_members), - size=normalized_size, - hash=hashlib.sha256(normalized_archive_bytes).hexdigest(), - ) + try: + manifest = SkillManifest( + name=name, + description=description, + entry_path=_SKILL_MD_NAME, + files=sorted(normalized_members), + size=normalized_size, + hash=hashlib.sha256(normalized_archive_bytes).hexdigest(), + ) + except ValidationError as exc: + raise self._manifest_validation_error(exc) from exc return NormalizedSkillPackage( manifest=manifest, archive_bytes=normalized_archive_bytes, @@ -124,6 +132,31 @@ class SkillPackageService: strip_prefix=strip_prefix, ) + @staticmethod + def _manifest_validation_error(exc: ValidationError) -> SkillPackageError: + first_error = exc.errors()[0] + loc = first_error["loc"] + field = loc[0] if loc else "manifest" + error_type = first_error["type"] + if field == "name": + code = "missing_skill_name" if error_type == "string_too_short" else "invalid_skill_name" + message = ( + "SKILL.md frontmatter name is required" + if code == "missing_skill_name" + else "SKILL.md frontmatter name must be lowercase letters, numbers, and hyphens only, " + "must not start or end with a hyphen, and must be at most 64 characters" + ) + return SkillPackageError(code, message, status_code=400) + if field == "description": + code = "missing_skill_description" if error_type == "string_too_short" else "invalid_skill_description" + message = ( + "SKILL.md frontmatter description is required" + if code == "missing_skill_description" + else f"SKILL.md frontmatter description must be at most {_MAX_SKILL_DESCRIPTION_LENGTH} characters" + ) + return SkillPackageError(code, message, status_code=400) + return SkillPackageError("invalid_skill_manifest", "SKILL.md frontmatter is invalid", status_code=400) + def _open_archive(self, *, content: bytes, filename: str) -> zipfile.ZipFile: self._check_extension(filename) if not content: @@ -282,13 +315,6 @@ class SkillPackageService: frontmatter = cls._parse_frontmatter(content) name = str(frontmatter.get("name") or "").strip() description = str(frontmatter.get("description") or "").strip() - if not name: - heading = _HEADING_RE.search(content) - name = heading.group(1).strip() if heading else "" - if not name: - raise SkillPackageError( - "missing_skill_name", "SKILL.md must declare a name (frontmatter or top heading)", status_code=400 - ) return name, description @staticmethod diff --git a/api/services/skill_management_service.py b/api/services/skill_management_service.py index ae2062d0314..85c4b990c21 100644 --- a/api/services/skill_management_service.py +++ b/api/services/skill_management_service.py @@ -84,6 +84,7 @@ _MAX_SKILLS_PER_WORKSPACE = 500 _MAX_AGENT_SKILLS = 20 _MAX_TAGS = 5 _MAX_TAG_LENGTH = 32 +_MAX_SKILL_DESCRIPTION_LENGTH = 1024 _UNTITLED_DISPLAY_NAME = "Untitled skill" _UNTITLED_SKILL_NAME_PREFIX = "untitled-skill" _UNTITLED_SKILL_DESCRIPTION = "Describe what this Skill does and when an Agent should use it." @@ -328,6 +329,16 @@ def validate_skill_name(name: str) -> str: return normalized +def validate_skill_description(description: str) -> str: + """Validate SKILL.md frontmatter description.""" + normalized = description.strip() + if not normalized: + raise ValueError("skill description must not be blank") + if len(normalized) > _MAX_SKILL_DESCRIPTION_LENGTH: + raise ValueError(f"skill description must be at most {_MAX_SKILL_DESCRIPTION_LENGTH} characters") + return normalized + + def normalize_skill_file_path(path: str) -> str: """Return a safe archive-relative file path.""" normalized = posixpath.normpath(path.strip().replace("\\", "/")) @@ -2433,7 +2444,14 @@ class SkillManagementService: "SKILL.md frontmatter description is required", details={"path": _SKILL_MD, "field": "description", "line": line}, ) - return description.strip()[:1024] + try: + return validate_skill_description(description) + except ValueError as exc: + raise SkillManagementServiceError( + "invalid_skill_description", + str(exc), + details={"path": _SKILL_MD, "field": "description", "line": line}, + ) from exc @staticmethod def _display_name_from_frontmatter(*, metadata: dict[str, Any], name: str) -> str: @@ -3060,5 +3078,6 @@ __all__ = [ "SkillRestorePayload", "SkillVersionUpdatePayload", "normalize_skill_file_path", + "validate_skill_description", "validate_skill_name", ] diff --git a/api/tests/unit_tests/services/agent/test_skill_package_service.py b/api/tests/unit_tests/services/agent/test_skill_package_service.py index 451069a3f9a..f634cfdda45 100644 --- a/api/tests/unit_tests/services/agent/test_skill_package_service.py +++ b/api/tests/unit_tests/services/agent/test_skill_package_service.py @@ -13,7 +13,7 @@ from services.agent import skill_package_service as skill_package_service_module from services.agent.skill_package_service import NormalizedSkillPackage, SkillPackageError, SkillPackageService _SKILL_MD = """--- -name: PDF Toolkit +name: pdf-toolkit description: Tools for working with PDF files. --- @@ -43,7 +43,7 @@ def _archive_members(content: bytes) -> list[str]: def test_valid_skill_normalizes_manifest(): manifest = _normalize({"SKILL.md": _SKILL_MD.encode(), "scripts/run.py": b"print('hi')\n"}).manifest - assert manifest.name == "PDF Toolkit" + assert manifest.name == "pdf-toolkit" assert manifest.description == "Tools for working with PDF files." assert manifest.entry_path == "SKILL.md" assert set(manifest.files) == {"SKILL.md", "scripts/run.py"} @@ -51,10 +51,10 @@ def test_valid_skill_normalizes_manifest(): assert len(manifest.hash) == 64 -def test_name_falls_back_to_heading_without_frontmatter(): - manifest = _normalize({"SKILL.md": b"# Heading Name\n\nbody"}).manifest - assert manifest.name == "Heading Name" - assert manifest.description == "" +def test_name_and_description_are_required_in_frontmatter(): + with pytest.raises(SkillPackageError) as exc_info: + _normalize({"SKILL.md": b"# heading-name\n\nbody"}) + assert exc_info.value.code == "missing_skill_name" def test_shallowest_skill_md_preferred_during_normalization(): @@ -155,7 +155,18 @@ def test_validate_and_normalize_strips_deeper_selected_skill_root(): ({"README.md": b"x"}, "skill.zip", "missing_skill_md"), ({"SKILL.md": _SKILL_MD.encode()}, "skill.tar", "unsupported_extension"), ({"SKILL.md": b""}, "skill.zip", "empty_skill_md"), - ({"SKILL.md": b"no name here"}, "skill.zip", "missing_skill_name"), + ({"SKILL.md": b"---\ndescription: valid\n---\n# no name here"}, "skill.zip", "missing_skill_name"), + ({"SKILL.md": b"---\nname: pdf-toolkit\n---\n# no description"}, "skill.zip", "missing_skill_description"), + ( + {"SKILL.md": b"---\nname: PDF Toolkit\ndescription: valid\n---\n# invalid name"}, + "skill.zip", + "invalid_skill_name", + ), + ( + {"SKILL.md": f"---\nname: pdf-toolkit\ndescription: {'x' * 1025}\n---\n# long".encode()}, + "skill.zip", + "invalid_skill_description", + ), ({"SKILL.md": b"\xff\xfenot utf8"}, "skill.zip", "skill_md_not_utf8"), ], ) @@ -224,10 +235,10 @@ def test_bad_frontmatter_yaml_rejected(): assert exc_info.value.code == "invalid_frontmatter" -def test_unterminated_frontmatter_falls_back_to_heading(): - # leading '---' with no closing fence -> no frontmatter, use the heading - manifest = _normalize({"SKILL.md": b"---\n# Heading Wins\nbody"}).manifest - assert manifest.name == "Heading Wins" +def test_unterminated_frontmatter_rejected(): + with pytest.raises(SkillPackageError) as exc_info: + _normalize({"SKILL.md": b"---\n# heading-wins\nbody"}) + assert exc_info.value.code == "missing_skill_name" def test_validate_and_normalize_rejects_files_outside_selected_skill_root(): diff --git a/api/tests/unit_tests/services/agent/test_skill_standardize_service.py b/api/tests/unit_tests/services/agent/test_skill_standardize_service.py index 922c018c729..2929b785077 100644 --- a/api/tests/unit_tests/services/agent/test_skill_standardize_service.py +++ b/api/tests/unit_tests/services/agent/test_skill_standardize_service.py @@ -20,7 +20,7 @@ _AGENT_ID = "22222222-2222-2222-2222-222222222222" _USER_ID = "33333333-3333-3333-3333-333333333333" _SKILL_MD = b"""--- -name: PDF Toolkit +name: pdf-toolkit description: Work with PDFs. --- @@ -121,7 +121,7 @@ def test_standardize_creates_drive_owned_toolfiles_and_commits_archive_manifest( assert skill_row.is_skill is True assert skill_row.skill_metadata is not None skill_metadata = DriveSkillMetadata.model_validate_json(skill_row.skill_metadata) - assert skill_metadata.name == "PDF Toolkit" + assert skill_metadata.name == "pdf-toolkit" assert skill_metadata.manifest_files == ["SKILL.md", "scripts/run.py"] assert archive_row.file_kind == AgentDriveFileKind.TOOL_FILE assert archive_row.file_id == archive_tool_file.id @@ -132,7 +132,7 @@ def test_standardize_creates_drive_owned_toolfiles_and_commits_archive_manifest( # The returned upload response carries only the drive-derived fields the UI needs. skill = result["skill"] assert skill["path"] == "pdf-toolkit" - assert skill["name"] == "PDF Toolkit" + assert skill["name"] == "pdf-toolkit" assert skill["archive_key"] == "pdf-toolkit/.DIFY-SKILL-FULL.zip" assert skill["skill_md_key"] == "pdf-toolkit/SKILL.md" assert result["manifest"]["entry_path"] == "SKILL.md" 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 9d4d205df62..b5afd9097d8 100644 --- a/api/tests/unit_tests/services/test_skill_management_service.py +++ b/api/tests/unit_tests/services/test_skill_management_service.py @@ -42,6 +42,7 @@ from services.skill_management_service import ( SkillRestorePayload, SkillVersionUpdatePayload, normalize_skill_file_path, + validate_skill_description, validate_skill_name, ) @@ -155,11 +156,18 @@ def _skill_md(name: str = "finance-sop", description: str = "Finance SOP", body: def test_validate_skill_name_rejects_underscores_and_double_hyphens() -> None: assert validate_skill_name("finance-sop") == "finance-sop" - for bad in ["finance_sop", "finance--sop", "-finance", "finance-"]: + for bad in ["finance_sop", "finance--sop", "-finance", "finance-", "Finance", "x" * 65]: with pytest.raises(ValueError): validate_skill_name(bad) +def test_validate_skill_description_rejects_blank_and_long_values() -> None: + assert validate_skill_description(" Finance SOP ") == "Finance SOP" + for bad in ["", " ", "x" * 1025]: + with pytest.raises(ValueError): + validate_skill_description(bad) + + def test_normalize_skill_file_path_rejects_escape_paths() -> None: assert normalize_skill_file_path("references//guide.md") == "references/guide.md" for bad in ["", "../x", "/etc/passwd", "a/\x00b"]: @@ -1170,6 +1178,25 @@ def test_replace_draft_tree_allows_blank_frontmatter_description_until_publish() assert exc_info.value.details == {"path": "SKILL.md", "field": "description", "line": 3} +def test_publish_rejects_too_long_frontmatter_description() -> None: + service = SkillManagementService(tool_file_manager=_FakeToolFileManager()) + created = service.create_skill(tenant_id=TENANT, user_id=USER, payload=SkillCreatePayload(name="finance-sop")) + + service.replace_draft_tree( + tenant_id=TENANT, + user_id=USER, + skill_id=created["id"], + payload=SkillDraftTreePayload( + files=[{"path": "SKILL.md", "content": _skill_md(description="x" * 1025, body="# Too long")}] + ), + ) + + with pytest.raises(SkillManagementServiceError) as exc_info: + service.publish_skill(tenant_id=TENANT, user_id=USER, skill_id=created["id"], payload=SkillPublishPayload()) + assert exc_info.value.code == "invalid_skill_description" + assert exc_info.value.details == {"path": "SKILL.md", "field": "description", "line": 3} + + def test_publish_reports_actual_frontmatter_name_line() -> None: service = SkillManagementService(tool_file_manager=_FakeToolFileManager()) created = service.create_skill(tenant_id=TENANT, user_id=USER, payload=SkillCreatePayload(name="finance-sop"))