Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 70 additions & 17 deletions astrbot/core/skills/skill_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@
SANDBOX_WORKSPACE_ROOT = "/workspace"
_SANDBOX_SKILLS_CACHE_VERSION = 1

_SKILL_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+$")
_SKILL_NAME_RE = re.compile(r"^[\w.-]+$")


def _normalize_skill_name(name: str | None) -> str:
raw = str(name or "")
return re.sub(r"\s+", "_", raw.strip())


def _default_sandbox_skill_path(name: str) -> str:
Expand Down Expand Up @@ -530,7 +535,13 @@ def delete_skill(self, name: str) -> None:
config["skills"].pop(name, None)
self._save_config(config)

def install_skill_from_zip(self, zip_path: str, *, overwrite: bool = True) -> str:
def install_skill_from_zip(
self,
zip_path: str,
*,
overwrite: bool = True,
skill_name_hint: str | None = None,
) -> str:
zip_path_obj = Path(zip_path)
if not zip_path_obj.exists():
raise FileNotFoundError(f"Zip file not found: {zip_path}")
Expand All @@ -547,15 +558,48 @@ def install_skill_from_zip(self, zip_path: str, *, overwrite: bool = True) -> st
if not file_names:
raise ValueError("Zip archive is empty.")

top_dirs = {
PurePosixPath(name).parts[0] for name in file_names if name.strip()
}
has_root_skill_md = any(
len(parts := PurePosixPath(name).parts) == 1
and parts[0] in {"SKILL.md", "skill.md"}
for name in file_names
)
root_mode = has_root_skill_md

if len(top_dirs) != 1:
raise ValueError("Zip archive must contain a single top-level folder.")
skill_name = next(iter(top_dirs))
if skill_name in {".", "..", ""} or not _SKILL_NAME_RE.match(skill_name):
raise ValueError("Invalid skill folder name.")
archive_skill_name = None
if skill_name_hint is not None:
archive_skill_name = _normalize_skill_name(skill_name_hint)
if archive_skill_name and not _SKILL_NAME_RE.fullmatch(
archive_skill_name
):
raise ValueError("Invalid skill name.")

if root_mode:
archive_hint = _normalize_skill_name(
archive_skill_name or zip_path_obj.stem
)
if not archive_hint or not _SKILL_NAME_RE.fullmatch(archive_hint):
raise ValueError("Invalid skill name.")
skill_name = archive_hint
else:
top_dirs = {
PurePosixPath(name).parts[0] for name in file_names if name.strip()
}
if len(top_dirs) != 1:
raise ValueError(
"Zip archive must contain a single top-level folder."
)
archive_root_name = next(iter(top_dirs))
archive_root_name_normalized = _normalize_skill_name(archive_root_name)
if archive_root_name in {".", "..", ""} or not _SKILL_NAME_RE.fullmatch(
archive_root_name_normalized
):
raise ValueError("Invalid skill folder name.")
if archive_skill_name:
if not _SKILL_NAME_RE.fullmatch(archive_skill_name):
raise ValueError("Invalid skill name.")
skill_name = archive_skill_name
else:
skill_name = archive_root_name_normalized

for name in names:
if not name:
Expand All @@ -565,24 +609,33 @@ def install_skill_from_zip(self, zip_path: str, *, overwrite: bool = True) -> st
parts = PurePosixPath(name).parts
if ".." in parts:
raise ValueError("Zip archive contains invalid relative paths.")
if parts and parts[0] != skill_name:
if (not root_mode) and parts and parts[0] != archive_root_name:
raise ValueError(
"Zip archive contains unexpected top-level entries."
)

if (
f"{skill_name}/SKILL.md" not in file_names
and f"{skill_name}/skill.md" not in file_names
):
raise ValueError("SKILL.md not found in the skill folder.")
if root_mode:
if "SKILL.md" not in file_names and "skill.md" not in file_names:
raise ValueError("SKILL.md not found in the skill folder.")
else:
if (
f"{archive_root_name}/SKILL.md" not in file_names
and f"{archive_root_name}/skill.md" not in file_names
):
raise ValueError("SKILL.md not found in the skill folder.")

with tempfile.TemporaryDirectory(dir=get_astrbot_temp_path()) as tmp_dir:
for member in zf.infolist():
member_name = member.filename.replace("\\", "/")
if not member_name or _is_ignored_zip_entry(member_name):
continue
zf.extract(member, tmp_dir)
src_dir = Path(tmp_dir) / skill_name
src_dir = (
Path(tmp_dir) if root_mode else Path(tmp_dir) / archive_root_name
)
normalized_path = _normalize_skill_markdown_path(src_dir)
if normalized_path is None:
raise ValueError("SKILL.md not found in the skill folder.")
_normalize_skill_markdown_path(src_dir)
if not src_dir.exists():
raise ValueError("Skill folder not found after extraction.")
Expand Down
96 changes: 84 additions & 12 deletions astrbot/dashboard/routes/skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import re
import shutil
import traceback
import uuid
from collections.abc import Awaitable, Callable
from pathlib import Path
from typing import Any
Expand Down Expand Up @@ -44,6 +43,17 @@ def _to_bool(value: Any, default: bool = False) -> bool:
_SKILL_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+$")


def _next_available_temp_path(temp_dir: str, filename: str) -> str:
stem = Path(filename).stem
suffix = Path(filename).suffix
candidate = filename
index = 1
while os.path.exists(os.path.join(temp_dir, candidate)):
candidate = f"{stem}_{index}{suffix}"
index += 1
return os.path.join(temp_dir, candidate)
Comment on lines +46 to +54
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Consider using a more robust mechanism than os.path.exists loops for picking a temp path.

The _next_available_temp_path loop has a TOCTTOU race between os.path.exists and file.save under concurrent uploads to the same temp_dir/filename. Prefer an OS-backed primitive like tempfile.NamedTemporaryFile(delete=False, dir=temp_dir, suffix=suffix) or tempfile.mkstemp to avoid collisions. If you need a readable base name, you can include the original stem in the generated prefix while still relying on the OS for uniqueness.

Suggested implementation:

import os
import re
import shutil
import tempfile
import traceback
from collections.abc import Awaitable, Callable
from pathlib import Path
from typing import Any
_SKILL_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+$")
def _next_available_temp_path(temp_dir: str, filename: str) -> str:
    stem = Path(filename).stem
    suffix = Path(filename).suffix

    # Use an OS-backed primitive to avoid TOCTTOU issues under concurrent uploads.
    # We include the original stem in the prefix for readability, while relying
    # on the OS for uniqueness.
    fd, path = tempfile.mkstemp(
        dir=temp_dir,
        prefix=f"{stem}_",
        suffix=suffix,
    )
    os.close(fd)
    return path



class SkillsRoute(Route):
def __init__(self, context: RouteContext, core_lifecycle) -> None:
super().__init__(context)
Expand Down Expand Up @@ -164,11 +174,24 @@ async def upload_skill(self):

temp_dir = get_astrbot_temp_path()
os.makedirs(temp_dir, exist_ok=True)
temp_path = os.path.join(temp_dir, filename)
skill_mgr = SkillManager()
temp_path = _next_available_temp_path(temp_dir, filename)
await file.save(temp_path)

skill_mgr = SkillManager()
skill_name = skill_mgr.install_skill_from_zip(temp_path, overwrite=True)
try:
try:
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False, skill_name_hint=Path(filename).stem
)
except TypeError:
# Backward compatibility for callers that do not accept skill_name_hint
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False
)
except Exception:
# Keep behavior consistent with previous implementation
# and bubble up install errors (including duplicates).
raise
Comment on lines +181 to +194
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The outer try/except in upload_skill that just re-raises looks unnecessary.

The outer try/except Exception: raise adds no behavior beyond normal exception propagation, since it neither adds context nor transforms the error. Unless you plan to add logging/metrics here soon, you can drop this wrapper to simplify the control flow without changing behavior.

Suggested change
try:
try:
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False, skill_name_hint=Path(filename).stem
)
except TypeError:
# Backward compatibility for callers that do not accept skill_name_hint
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False
)
except Exception:
# Keep behavior consistent with previous implementation
# and bubble up install errors (including duplicates).
raise
try:
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False, skill_name_hint=Path(filename).stem
)
except TypeError:
# Backward compatibility for callers that do not accept skill_name_hint
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False
)


try:
await sync_skills_to_active_sandboxes()
Expand Down Expand Up @@ -208,6 +231,7 @@ async def batch_upload_skills(self):

succeeded = []
failed = []
skipped = []
skill_mgr = SkillManager()
temp_dir = get_astrbot_temp_path()
os.makedirs(temp_dir, exist_ok=True)
Expand All @@ -226,14 +250,42 @@ async def batch_upload_skills(self):
)
continue

temp_path = os.path.join(
temp_dir, f"batch_{uuid.uuid4().hex}_{filename}"
)
temp_path = _next_available_temp_path(temp_dir, filename)
await file.save(temp_path)

skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=True
)
try:
skill_name = skill_mgr.install_skill_from_zip(
temp_path,
overwrite=False,
skill_name_hint=Path(filename).stem,
)
except TypeError:
# Backward compatibility for monkeypatched implementations in tests
try:
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False
)
except FileExistsError:
skipped.append(
{
"filename": filename,
"name": Path(filename).stem,
"error": "Skill already exists.",
}
)
skill_name = None
except FileExistsError:
skipped.append(
{
"filename": filename,
"name": Path(filename).stem,
"error": "Skill already exists.",
}
)
skill_name = None

if skill_name is None:
continue
succeeded.append({"filename": filename, "name": skill_name})

except Exception as e:
Expand All @@ -255,8 +307,10 @@ async def batch_upload_skills(self):

total = len(file_list)
success_count = len(succeeded)
skipped_count = len(skipped)
failed_count = len(failed)

if success_count == total:
if failed_count == 0 and success_count == total:
message = f"All {total} skill(s) uploaded successfully."
return (
Response()
Expand All @@ -265,18 +319,35 @@ async def batch_upload_skills(self):
"total": total,
"succeeded": succeeded,
"failed": failed,
"skipped": skipped,
},
message,
)
.__dict__
)
if failed_count == 0 and success_count == 0:
message = f"All {total} file(s) were skipped."
return (
Response()
.ok(
{
"total": total,
"succeeded": succeeded,
"failed": failed,
"skipped": skipped,
},
message,
)
.__dict__
)
if success_count == 0:
if success_count == 0 and skipped_count == 0:
message = f"Upload failed for all {total} file(s)."
resp = Response().error(message)
resp.data = {
"total": total,
"succeeded": succeeded,
"failed": failed,
"skipped": skipped,
}
return resp.__dict__

Expand All @@ -288,6 +359,7 @@ async def batch_upload_skills(self):
"total": total,
"succeeded": succeeded,
"failed": failed,
"skipped": skipped,
},
message,
)
Expand Down
9 changes: 9 additions & 0 deletions dashboard/src/components/extension/SkillsSection.vue
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,7 @@ export default {
const applyUploadResults = (attemptedItems, payload) => {
const succeededMap = buildResultMap(payload?.succeeded);
const failedMap = buildResultMap(payload?.failed);
const skippedMap = buildResultMap(payload?.skipped);

for (const item of attemptedItems) {
const successEntry = takeFirstMatch(succeededMap, item.filenameKey);
Expand All @@ -911,6 +912,14 @@ export default {
continue;
}

const skippedEntry = takeFirstMatch(skippedMap, item.filenameKey);
if (skippedEntry) {
item.status = STATUS_SKIPPED;
item.validationMessage =
skippedEntry.error || tm("skills.validationDuplicate");
continue;
}

const failedEntry = takeFirstMatch(failedMap, item.filenameKey);
if (failedEntry) {
item.status = STATUS_ERROR;
Expand Down
2 changes: 1 addition & 1 deletion dashboard/src/i18n/locales/en-US/features/extension.json
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@
"emptyHint": "Upload a Skills zip to get started",
"uploadDialogTitle": "Upload Skills",
"uploadHint": "Upload multiple zip skill packages or drag them in. The system validates the structure automatically and shows a result for each file.",
"structureRequirement": "The most common failure is an invalid archive structure. Each zip must contain exactly one top-level folder such as `skillname/`, and that folder must include `SKILL.md`.",
"structureRequirement": "The archive supports multiple skills folders.",
"abilityMultiple": "Upload multiple zip files at once",
"abilityValidate": "Validate `SKILL.md` automatically",
"abilitySkip": "Automatically skip duplicate files.",
Expand Down
2 changes: 1 addition & 1 deletion dashboard/src/i18n/locales/ru-RU/features/extension.json
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@
"emptyHint": "Пожалуйста, загрузите архив с навыками",
"uploadDialogTitle": "Загрузка навыков",
"uploadHint": "Поддерживается массовая загрузка zip-архивов. Вы также можете перетащить файлы в это окно. Система автоматически проверит структуру каждого архива.",
"structureRequirement": "Архив должен содержать одну корневую папку (например, `skillname/`), внутри которой обязательно должен находиться файл `SKILL.md`.",
"structureRequirement": "Поддерживаются архивы с несколькими папками skills.",
"abilityMultiple": "Поддержка массовой загрузки",
"abilityValidate": "Автопроверка `SKILL.md`",
"abilitySkip": "Пропуск дубликатов",
Expand Down
2 changes: 1 addition & 1 deletion dashboard/src/i18n/locales/zh-CN/features/extension.json
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@
"emptyHint": "请上传 Skills 压缩包",
"uploadDialogTitle": "上传 Skills",
"uploadHint": "支持批量上传 zip 技能包,也支持拖拽批量上传 zip 技能包。系统会自动校验目录结构,并给出逐个文件的结果。",
"structureRequirement": "常见失败原因是压缩包结构不正确。每个 zip 必须只包含一个顶层目录,例如 `skillname/`,且该目录下必须存在 `SKILL.md`。",
"structureRequirement": "支持压缩包内含多个 skills 文件夹。",
"abilityMultiple": "支持一次上传多个zip文件",
"abilityValidate": "自动校验 `SKILL.md`",
"abilitySkip": "自动跳过重复文件",
Expand Down
Loading